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 <[email protected]> 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/[email protected]?part=8
