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

[sashiko-bot@kernel.org]

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

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