Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] famfs_fuse_write_iter and famfs_fuse_read_iter completely bypass mandatory VFS locking, security privilege stripping, and POSIX semantics. - [High] famfs_fuse_mmap fails to set the VM_MIXEDMAP flag, guaranteeing a BUG_ON() crash during DAX page faults. - [High] Explicitly returning -EIO for splice operations on famfs files breaks standard system calls like sendfile() and splice(). - [High] 32-bit architectures suffer from silent data truncation due to the use of size_t and off_t for 64-bit file offsets and sizes. - [Medium] Concurrent, lockless modification of `meta->error` in `famfs_file_bad()` creates a data race. --
--- Patch [6]: [PATCH V11 6/9] famfs_fuse: Plumb dax iomap and fuse read/write/mmap --- commit 26226f89d9106affa50e5f7fc0a4be4656c4531c Author: John Groves <[email protected]> famfs_fuse: Plumb dax iomap and fuse read/write/mmap This commit adds read, write, and mmap support for famfs files using the dev_dax_iomap interface. It handles file offset resolution and page faults via DAX upcalls. > diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c > index a2a7dd631dc0f..ac56317944d93 100644 > --- a/fs/fuse/famfs.c > +++ b/fs/fuse/famfs.c [ ... ] > +static int > +famfs_fileofs_to_daxofs(struct inode *inode, struct iomap *iomap, > + loff_t file_offset, off_t len, unsigned int flags) > +{ [Severity: High] Does defining the len parameter as off_t truncate the mapping length on 32-bit systems? [ ... ] > +const struct vm_operations_struct famfs_file_vm_ops = { > + .fault = famfs_filemap_fault, > + .huge_fault = famfs_filemap_huge_fault, > + .map_pages = filemap_map_pages, [Severity: High] Is it correct to assign a page-cache-specific mapping function here? Since DAX mappings bypass the page cache, using filemap_map_pages might lead to unexpected behavior. > + .page_mkwrite = famfs_filemap_mkwrite, > + .pfn_mkwrite = famfs_filemap_mkwrite, > +}; [ ... ] > +static int > +famfs_file_bad(struct inode *inode) > +{ > + struct fuse_inode *fi = get_fuse_inode(inode); > + struct famfs_file_meta *meta = fi->famfs_meta; > + size_t i_size = i_size_read(inode); > + > + if (!meta) { > + pr_err("%s: un-initialized famfs file\n", __func__); > + return -EIO; > + } > + if (meta->error) { > + pr_debug("%s: previously detected metadata errors\n", __func__); > + return -EIO; > + } > + if (i_size != meta->file_size) { > + pr_warn("%s: i_size overwritten from %ld to %ld\n", > + __func__, meta->file_size, i_size); > + meta->error = true; [Severity: Medium] Can concurrent I/O operations cause a data race on meta->error? Since read and write paths can execute without the inode lock, multiple threads might modify this flag simultaneously. > + return -ENXIO; > + } [ ... ] > +static ssize_t > +famfs_fuse_rw_prep(struct kiocb *iocb, struct iov_iter *ubuf) > +{ > + struct inode *inode = iocb->ki_filp->f_mapping->host; > + size_t i_size = i_size_read(inode); [Severity: High] Might reading i_size_read() into a size_t truncate the 64-bit value on 32-bit platforms? This would break I/O bounds checking for files larger than 4GB. This also applies to the file_size field in the famfs_file_meta structure if it is similarly defined as size_t. > + size_t count = iov_iter_count(ubuf); > + size_t max_count; > + ssize_t rc; [ ... ] > +ssize_t > +famfs_fuse_read_iter(struct kiocb *iocb, struct iov_iter *to) > +{ > + ssize_t rc; > + > + rc = famfs_fuse_rw_prep(iocb, to); > + if (rc) > + return rc; > + > + if (!iov_iter_count(to)) > + return 0; > + > + rc = dax_iomap_rw(iocb, to, &famfs_iomap_ops); [Severity: Critical] Does this read path need to acquire inode_lock_shared()? The standard VFS read paths typically acquire it before calling dax_iomap_rw(). > + > + file_accessed(iocb->ki_filp); > + return rc; > +} > + > +ssize_t > +famfs_fuse_write_iter(struct kiocb *iocb, struct iov_iter *from) > +{ > + ssize_t rc; > + > + rc = famfs_fuse_rw_prep(iocb, from); > + if (rc) > + return rc; > + > + if (!iov_iter_count(from)) > + return 0; > + > + return dax_iomap_rw(iocb, from, &famfs_iomap_ops); > +} [Severity: Critical] Does this write path omit mandatory VFS security and integrity checks? Calling dax_iomap_rw() directly without acquiring inode_lock() bypasses generic_write_checks() and file_remove_privs(), which could allow unprivileged modification of setuid/setgid files and bypass O_APPEND restrictions. > +int > +famfs_fuse_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct inode *inode = file_inode(file); > + ssize_t rc; > + > + rc = famfs_file_bad(inode); > + if (rc) > + return rc; > + > + file_accessed(file); > + vma->vm_ops = &famfs_file_vm_ops; > + vm_flags_set(vma, VM_HUGEPAGE); [Severity: High] Could this trigger a kernel panic during DAX page faults? The vmf_insert_mixed() path expects VM_MIXEDMAP or VM_PFNMAP to be set on the vma, but VM_MIXEDMAP appears to be omitted here. > + return 0; > +} [ ... ] > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index b4e7b6a645870..2435a79cbb4a3 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c [ ... ] > @@ -1896,9 +1900,13 @@ static ssize_t fuse_splice_read(struct file *in, > loff_t *ppos, > unsigned int flags) > { > struct fuse_file *ff = in->private_data; > + struct inode *inode = file_inode(in); > + struct fuse_inode *fi = get_fuse_inode(inode); > > /* FOPEN_DIRECT_IO overrides FOPEN_PASSTHROUGH */ > - if (fuse_file_passthrough(ff) && !(ff->open_flags & FOPEN_DIRECT_IO)) > + if (fuse_file_famfs(fi)) > + return -EIO; /* famfs does not use the page cache... */ [Severity: High] Will returning -EIO break standard system calls like splice() and sendfile() for famfs files? Should this fall back to iter_file_splice_read() or copy_splice_read() instead? > + else if (fuse_file_passthrough(ff) && !(ff->open_flags & > FOPEN_DIRECT_IO)) > return fuse_passthrough_splice_read(in, ppos, pipe, len, flags); > else > return filemap_splice_read(in, ppos, pipe, len, flags); > @@ -1908,9 +1916,13 @@ static ssize_t fuse_splice_write(struct > pipe_inode_info *pipe, struct file *out, > loff_t *ppos, size_t len, unsigned int flags) > { > struct fuse_file *ff = out->private_data; > + struct inode *inode = file_inode(out); > + struct fuse_inode *fi = get_fuse_inode(inode); > > /* FOPEN_DIRECT_IO overrides FOPEN_PASSTHROUGH */ > - if (fuse_file_passthrough(ff) && !(ff->open_flags & FOPEN_DIRECT_IO)) > + if (fuse_file_famfs(fi)) > + return -EIO; /* famfs does not use the page cache... */ [Severity: High] Does hardcoding -EIO here also break splice writes? Falling back to iter_file_splice_write() might be required to keep standard system calls functioning. > + else if (fuse_file_passthrough(ff) && !(ff->open_flags & > FOPEN_DIRECT_IO)) > return fuse_passthrough_splice_write(pipe, out, ppos, len, > flags); > else > return iter_file_splice_write(pipe, out, ppos, len, flags); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000...@email.amazonses.com?part=6
