Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Medium] Hardcoded NULL iomap_ops passed to DAX functions will cause a kernel panic - [Medium] Missing file_modified() call allows bypassing security privileges on write - [Medium] Missing generic_write_sync() silently breaks DAX synchronous writes - [Medium] Missing FOP_MMAP_SYNC flag prevents MAP_SYNC usage - [Medium] mprotect() can bypass the FAMFS_OPT_WRITE gate to gain writable mappings - [Medium] Missing lock serialization between DAX page faults and file truncation - [Low] Setting .map_pages to filemap_map_pages is invalid for DAX mappings --
--- Patch [6]: [PATCH V12 06/12] famfs: Introduce mmap and VM fault handling --- commit a41970b8d49174c2162aaa9777d2fc2abc414431 Author: John Groves <[email protected]> famfs: Introduce mmap and VM fault handling This commit adds vm_operations, famfs_mmap(), and fault handlers to the famfs filesystem. It sets up the groundwork for memory mapping DAX files, though it is still missing iomap_ops and mapping resolution. > diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c > index e192b573c51f7..678f2035fd5fa 100644 > --- a/fs/famfs/famfs_file.c > +++ b/fs/famfs/famfs_file.c > @@ -16,6 +16,75 @@ > > #include "famfs_internal.h" > > +/********************************************************************* > + * vm_operations > + */ > +static vm_fault_t > +__famfs_filemap_fault(struct vm_fault *vmf, unsigned int order, > + bool write_fault) > +{ > + struct inode *inode = file_inode(vmf->vma->vm_file); > + struct super_block *sb = inode->i_sb; > + struct famfs_fs_info *fsi = sb->s_fs_info; > + vm_fault_t ret; > + unsigned long pfn; > + > + if (fsi->deverror) > + return VM_FAULT_SIGBUS; > + > + if (!IS_DAX(file_inode(vmf->vma->vm_file))) { > + pr_err("%s: file not marked IS_DAX!!\n", __func__); > + return VM_FAULT_SIGBUS; > + } > + > + if (write_fault) { > + sb_start_pagefault(inode->i_sb); > + file_update_time(vmf->vma->vm_file); > + } > + > + ret = dax_iomap_fault(vmf, order, &pfn, NULL, NULL /*&famfs_iomap_ops > */); [Severity: Medium] Will this hardcoded NULL pointer for the iomap_ops argument cause a kernel panic? The core DAX subsystem unconditionally dereferences the ops pointer (for example, by calling ops->iomap_begin()). This also appears to happen in famfs_dax_read_iter() and famfs_dax_write_iter() when calling dax_iomap_rw(). [Severity: Medium] Is there missing lock serialization between DAX page faults and file truncation here? Calling the DAX page fault handler without holding an internal filesystem lock (like i_mmap_sem) might allow concurrent truncation and page faults to race, mapping DAX pages out-of-bounds or into truncated regions. > + if (ret & VM_FAULT_NEEDDSYNC) > + ret = dax_finish_sync_fault(vmf, order, pfn); > + > + if (write_fault) > + sb_end_pagefault(inode->i_sb); > + > + return ret; > +} > + [ ... ] > +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: Low] Is it valid to assign filemap_map_pages to a DAX filesystem? filemap_map_pages is designed to map standard page cache folios, but DAX filesystems use value entries in the xarray. Since modern kernels safely skip these via xa_is_value(), iterating the xarray to map nothing wastes CPU cycles and appears to be an architectural violation. > + .page_mkwrite = famfs_filemap_mkwrite, > + .pfn_mkwrite = famfs_filemap_mkwrite, > +}; > + > /********************************************************************* > * file_operations > */ > @@ -117,6 +186,36 @@ famfs_dax_write_iter(struct kiocb *iocb, struct iov_iter > *from) [Severity: Medium] Does the I/O performed in famfs_dax_write_iter() miss a preceding call to file_modified()? If so, when an unprivileged user writes to a file, the setuid and setgid bits are not stripped, and the mtime/ctime timestamps are not updated, violating POSIX security and semantics. [Severity: Medium] Does famfs_dax_write_iter() return the result of the write without calling generic_write_sync() to perform the necessary cache flushes? Without this, data written with synchronous flags (like O_SYNC or O_DSYNC) will not be flushed from the CPU cache to persistent media, leading to silent data loss upon power failure despite user requests for persistence. > return rc; > } > > +static int > +famfs_file_mmap(struct file *file, struct vm_area_struct *vma) > +{ > + struct inode *inode = file_inode(file); > + struct super_block *sb = inode->i_sb; > + struct famfs_fs_info *fsi = sb->s_fs_info; > + ssize_t rc; > + > + if (fsi->deverror) > + return -ENODEV; > + > + /* > + * Gate shared-writable mappings on FAMFS_OPT_WRITE. This is best > + * effort: clearing the bit blocks new writable mappings and write(), > + * but does not revoke mappings that already exist. > + */ > + if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) && > + !famfs_opt_enabled(fsi, FAMFS_OPT_WRITE)) [Severity: Medium] Does this missing check for VM_MAYWRITE leave the VMA vulnerable to mprotect upgrades? If a user maps the file with PROT_READ when FAMFS_OPT_WRITE is disabled, they could later bypass the access control and obtain a writable mapping by calling mprotect() to PROT_WRITE. > + return -EPERM; > + > + rc = famfs_file_invalid(inode); > + if (rc) > + return (int)rc; > + > + file_accessed(file); > + vma->vm_ops = &famfs_file_vm_ops; > + vm_flags_set(vma, VM_HUGEPAGE); > + return 0; > +} > + > const struct file_operations famfs_file_operations = { > .owner = THIS_MODULE, > > @@ -124,7 +223,7 @@ const struct file_operations famfs_file_operations = { > .write_iter = famfs_dax_write_iter, > .read_iter = famfs_dax_read_iter, > .unlocked_ioctl = NULL /*famfs_file_ioctl*/, > - .mmap = NULL /* famfs_file_mmap */, > + .mmap = famfs_file_mmap, [Severity: Medium] Does the file operations structure lack the required .fop_flags = FOP_MMAP_SYNC? Without this flag, the VFS will reject mmap() requests that include MAP_SYNC | MAP_SHARED_VALIDATE with -EOPNOTSUPP, breaking the core DAX use-case of userspace flushing without fsync. > > /* Force PMD alignment for mmap */ > .get_unmapped_area = thp_get_unmapped_area, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000...@email.amazonses.com?part=6
