Re: [PATCH v4 8/9] fuse: add extent map I/O support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fuse: silent data loss on fsync for DAX extent-mapped files
- [High] fuse: ABBA deadlock between pipe_lock and inode lock in splice write
- [High] fuse: concurrent EXTMAP DAX faults and truncate/punch_hole race due to missing lock
--

commit 5f3e8f02d15211d77c4f619bc19ce42ddb86c547
Author: Miklos Szeredi <mszeredi@redhat.com>

fuse: add extent map I/O support

Wire up read, write, splice and mmap operations for extent-mapped files
through the iomap/DAX infrastructure. When a passthrough file is opened
with an EXTMAP backing, I/O is dispatched to the dax devices referenced
by the extent map.

> diff --git a/fs/fuse/ext_map.c b/fs/fuse/ext_map.c
> index afac5de51c802..744940cbf36e1 100644
> --- a/fs/fuse/ext_map.c
> +++ b/fs/fuse/ext_map.c
[ ... ]
> +static vm_fault_t fuse_ext_map_huge_fault(struct vm_fault *vmf, unsigned int order)
> +{
> +	struct inode *inode = file_inode(vmf->vma->vm_file);
> +	bool write_fault = (vmf->flags & FAULT_FLAG_WRITE) && (vmf->vma->vm_flags & VM_SHARED);
> +	vm_fault_t ret;
> +	unsigned long pfn;
> +
> +	if (!IS_ENABLED(CONFIG_FS_DAX))
> +		return VM_FAULT_SIGBUS;
> +
> +	if (WARN_ON_ONCE(!IS_DAX(inode)))
> +		return VM_FAULT_SIGBUS;
> +
> +	if (write_fault) {
> +		sb_start_pagefault(inode->i_sb);
> +		file_update_time(vmf->vma->vm_file);
> +	}
> +
> +	filemap_invalidate_lock_shared(inode->i_mapping);

[Severity: High]
Does this allow concurrent truncate or punch_hole operations to race with page
faults?

The page fault handler acquires filemap_invalidate_lock_shared() here,
expecting writers to hold it exclusively during invalidation. However, writer
paths like fuse_do_setattr() and fuse_file_fallocate() check FUSE_IS_VDAX()
to decide whether to acquire the exclusive filemap_invalidate_lock. For EXTMAP
DAX files, FUSE_IS_VDAX() evaluates to false, causing the exclusive lock
acquisition to be skipped. Consequently, page faults are not blocked during
truncate_pagecache() or truncate_pagecache_range().

Can this leave stale DAX entries in the mapping tree, leading to
use-after-free of storage blocks when memory-mapped I/O accesses blocks that
have been freed or punched out?

> +
> +	ret = dax_iomap_fault(vmf, order, &pfn, NULL, &fuse_ext_map_iomap_ops);
> +	if (ret & VM_FAULT_NEEDDSYNC)
> +		ret = dax_finish_sync_fault(vmf, order, pfn);
> +
> +	filemap_invalidate_unlock_shared(inode->i_mapping);
> +
> +	if (write_fault)
> +		sb_end_pagefault(inode->i_sb);
> +
> +	return ret;
> +}
[ ... ]
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index b5bef7b9e466e..77d7a54066e47 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -551,6 +551,17 @@ static int fuse_fsync(struct file *file, loff_t start, loff_t end,
>  	if (fuse_is_bad(inode))
>  		return -EIO;
>  
> +	if (IS_DAX(inode) && !fuse_inode_vdax(inode)) {
> +		/*
> +		 * Note: the DAX fault path calls this via dax_finish_sync_fault()
> +		 * and taking inode lock in that context is prohibited.
> +		 *
> +		 * FIXME: need to flush CPU caches for the DAX memory range
> +		 * FIXME: need to sync metadata to the server
> +		 */
> +		return 0;

[Severity: High]
Can this cause silent data loss on fsync() for DAX extent-mapped files?

When an application calls fsync() on a FUSE DAX extent-mapped file, this
bypasses flushing the CPU caches for the DAX memory range or syncing metadata
to the server, while still returning 0 (success).

Because the operation falsely reports success, applications relying on fsync()
or MAP_SYNC (via dax_finish_sync_fault()) to ensure data durability will not
take fallback actions. Could this lead to silent data loss on a power failure
or crash?

> +	}
> +
>  	inode_lock(inode);
>  
>  	/*
> diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> index e9ab1aea34e27..040817ad76e94 100644
> --- a/fs/fuse/passthrough.c
> +++ b/fs/fuse/passthrough.c
[ ... ]
> @@ -74,10 +73,13 @@ ssize_t fuse_passthrough_write_iter(struct kiocb *iocb,
>  	if (!count)
>  		return 0;
>  
> -	inode_lock(inode);
> +	guard(rwsem_write)(&inode->i_rwsem);
> +
> +	if (!backing_file)
> +		return fuse_ext_map_write_iter(iocb, iter);

[Severity: High]
Does this introduce an ABBA deadlock between pipe_lock and the inode lock?

For normal passthrough, fuse_passthrough_splice_write() acquires the inode
lock first, and then calls backing_file_splice_write(), which ultimately
acquires pipe_lock() via the backing file's splice_write callback.

However, the EXTMAP DAX path added below for splice_write() directly calls
iter_file_splice_write(), which acquires pipe_lock() first. That function then
invokes fuse_passthrough_write_iter() where the inode lock is acquired here
via guard(rwsem_write).

If a malicious FUSE server provides a normal passthrough backing for one open
and an EXTMAP backing for another open of the same inode, and two threads
perform splice writes using the same pipe and the two different file
descriptors, it results in a literal ABBA deadlock. Even operating on
different inodes would trigger a lockdep lock inversion splat.

> +
>  	ret = backing_file_write_iter(backing_file, iter, iocb, iocb->ki_flags,
>  				      &ctx);
> -	inode_unlock(inode);
[ ... ]
> @@ -123,6 +128,9 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>  	pr_debug("%s: backing_file=0x%p, pos=%lld, len=%zu, flags=0x%x\n", __func__,
>  		 backing_file, *ppos, len, flags);
>  
> +	if (!backing_file)
> +		return iter_file_splice_write(pipe, out, ppos, len, flags);
> +
>  	inode_lock(inode);
>  	init_sync_kiocb(&iocb, out);
>  	iocb.ki_pos = *ppos;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008112004.1899560-1-mszeredi@redhat.com?part=8