Re: [PATCH V14 07/12] famfs: MAP_CREATE ioctl and fmap ingest (ABI 44)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing memory barriers when publishing and reading `inode->i_private` allows concurrent readers to see uninitialized metadata.
- [Medium] Reserved fields in the UAPI structs are not validated to be zero, breaking forward compatibility.
--

commit 01eb70d8c2de27c662f746e15fb2d3e809666490
Author: John Groves <John@Groves.net>

famfs: MAP_CREATE ioctl and fmap ingest (ABI 44)
    
Add the famfs file ioctl handler (FAMFSIOC_NOP, FAMFSIOC_MAP_CREATE) and
the KABI-44 self-describing fmap message: the wire ABI in famfs_ioctl.h
(famfs_ioc_fmap_header plus the simple and interleaved extent structs), the
in-core famfs_file_meta, and famfs_file_init_dax(), which copies the
message in, parses both the simple-extent and interleaved (striped) wire
forms into inode->i_private, and sets S_DAX.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> index 9c1cd2f674893..f099e0d08673f 100644
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c
[ ... ]
> @@ -13,9 +13,377 @@
[ ... ]
> +static int
> +famfs_file_init_dax(struct file *file, void __user *arg)
> +{
> +	struct famfs_ioc_fmap_header fmh;
[ ... ]
> +	if (copy_from_user(&fmh, arg, sizeof(fmh)))
> +		return -EFAULT;
> +
> +	if (fmh.fmap_version != FAMFS_FMAP_VERSION)
> +		return -EINVAL;
> +	if (fmh.fmap_size < sizeof(fmh))
> +		return -EINVAL;

[Severity: Medium]
Are the reserved and reserved1 fields in famfs_ioc_fmap_header validated
to be zero here? 

If userspace passes uninitialized or garbage data in these fields, the kernel
will blindly accept it. This could prevent the kernel from safely assigning
meaning to these fields in the future without breaking backward compatibility,
since it won't be able to distinguish between older applications passing
garbage and newer applications intentionally using the flags. 

The reserved fields in famfs_ioc_simple_ext and famfs_ioc_iext also appear
to lack zero-validation.

> +	if (fmh.fmap_size > FAMFS_FMAP_MSG_MAX)
> +		return -EFBIG;
[ ... ]
> +	/* Publish the famfs metadata on inode->i_private */
> +	inode_lock(inode);
> +	if (inode->i_private) {
> +		rc = -EEXIST; /* file already has famfs metadata */
> +	} else {
> +		inode->i_private = meta;
> +		i_size_write(inode, meta->file_size);

[Severity: High]
Does this assignment to inode->i_private need a release memory barrier?

This plain store publishes the structure without a release barrier, allowing
prior initialization stores to be reordered after it. A concurrent reader
could observe a non-NULL inode->i_private pointer but read uninitialized or
stale metadata fields.

For instance, a concurrent mmap() on the same file calls famfs_file_mmap()
which in turn calls famfs_file_invalid(inode). Because famfs_file_mmap()
does not hold inode_lock, famfs_file_invalid() will execute a plain load
(struct famfs_file_meta *meta = inode->i_private;) without an acquire
barrier. 

If it reads uninitialized data like meta->file_size, it could fail the 
i_size != meta->file_size check, which permanently sets meta->error = true,
thereby permanently bricking the file mapping.

Should smp_store_release() and smp_load_acquire() be used here to enforce
the required memory ordering?

> +		inode->i_flags |= S_DAX;
> +		meta = NULL; /* owned by the inode now */
> +		rc = 0;
> +	}
> +	inode_unlock(inode);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000000@email.amazonses.com?part=7