Re: [PATCH V11 6/9] famfs_fuse: Plumb dax iomap and fuse read/write/mmap

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