Re: [PATCH V14 06/12] famfs: Introduce mmap and VM fault handling

[sashiko-bot@kernel.org]

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 <john@groves.net>

    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-000000@email.amazonses.com?part=6