Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Modifying VMA flags in a legacy .mmap callback breaks VMA merging, leading to memory fragmentation. --
commit f0dd25d565afad641be49c7b1903722fa578648a Author: John Groves <[email protected]> famfs: Introduce mmap and VM fault handling This commit adds vm_operations, plus famfs_mmap() and fault handlers. It is still missing iomap_ops, iomap mapping resolution, and famfs_ioctl() for setting up file-to-memory mappings. > diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c > index 1369fe1824bc8..9c1cd2f674893 100644 > --- a/fs/famfs/famfs_file.c > +++ b/fs/famfs/famfs_file.c [ ... ] > @@ -120,14 +190,51 @@ famfs_dax_write_iter(struct kiocb *iocb, struct > iov_iter *from) > 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. Reject a mapping > + * that is already writable, and strip VM_MAYWRITE from a read-only > + * shared mapping so a later mprotect(PROT_WRITE) cannot upgrade it. > + * This is best effort: it does not revoke mappings that already exist. > + */ > + if ((vma->vm_flags & VM_SHARED) && > + !famfs_opt_enabled(fsi, FAMFS_OPT_WRITE)) { > + if (vma->vm_flags & VM_WRITE) > + return -EPERM; > + vm_flags_clear(vma, VM_MAYWRITE); [Severity: Medium] Does clearing VM_MAYWRITE in the legacy f_op->mmap callback break VMA merging? Since this occurs post-merge attempt, these modified flags diverge from the initially proposed flags. Subsequent adjacent mmaps of the same file with identical protection will fail to merge. Should this driver implement f_op->mmap_prepare() instead to modify VMA flags before the merge attempt? > + } > + > + 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); [Severity: Medium] Will setting VM_HUGEPAGE here guarantee that adjacent default mappings fail to merge? Like VM_MAYWRITE, modifying this flag post-merge attempt prevents the core MM subsystem from successfully merging new mappings with existing ones. Should VM_HUGEPAGE also be set via f_op->mmap_prepare() instead? > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000...@email.amazonses.com?part=6
