Re: [PATCH V11 4/9] famfs_fuse: Create files with famfs fmaps

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] File size and extent lengths are truncated to 32 bits on 32-bit architectures, leading to file size corruption and validation bypass.
- [High] Premature publication of DAX metadata allows concurrent threads to observe an incomplete initialization state.
- [High] Modifying `inode->i_flags` directly with bitwise operations bypasses the required `inode->i_lock` spinlock, causing lost updates.
- [Medium] Unvalidated FUSE server input passed directly to `BIT_ULL()` causes a shift-out-of-bounds, leading to Undefined Behavior.
- [Medium] The `famfs_file_meta` and its extent array are allocated without `__GFP_ACCOUNT`, allowing unbounded kernel memory consumption.
- [Low] `famfs_meta_set()` incorrectly returns `struct fuse_backing *` instead of `void *`.
- [Low] Ineffective concurrency check using `cmpxchg()` on a local stack variable.
--

--- Patch [4]: [PATCH V11 4/9] famfs_fuse: Create files with famfs fmaps ---
commit 293d4aeecd8b2f11982375cde3643693ae41f5d0
Author: John Groves <[email protected]>

famfs_fuse: Create files with famfs fmaps

This commit interprets the fmap retrieved by GET_FMAP into the in-memory
metadata that later patches use to resolve read/write/mmap directly to dax.
It adds the fmap wire format, in-memory structures, and metadata
allocation logic.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index 80e6640ac970b..8f7ee7d6151bb 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
[ ... ]
> @@ -14,13 +14,244 @@
[ ... ]
> +	struct famfs_file_meta *meta __free(__famfs_meta_free) = kzalloc(sizeof(*meta), GFP_KERNEL);

[Severity: Medium]
Is it intentional to omit the __GFP_ACCOUNT flag here? A single DAX file can
have up to 32,768 extents, and unprivileged containerized processes could
repeatedly open FUSE DAX files to trigger these allocations. Adding
__GFP_ACCOUNT would ensure these allocations are charged to the memory cgroup.

> +
> +	if (!meta)
> +		return -ENOMEM;
> +
> +	meta->error = false;
> +	meta->file_type = fmh->file_type;
> +	meta->file_size = fmh->file_size;
> +
> +	switch (fmh->ext_type) {
> +	case FUSE_FAMFS_EXT_SIMPLE: {
> +		struct fuse_famfs_simple_ext *se_in;
> +
> +		se_in = fmap_buf + next_offset;
> +
> +		/* Move past simple extents */
> +		next_offset += fmh->nextents * sizeof(*se_in);
> +		if (next_offset > fmap_buf_size) {
> +			pr_err("%s:%d: fmap_buf underflow offset/size %ld/%ld\n",
> +			       __func__, __LINE__, next_offset, fmap_buf_size);
> +			return -EINVAL;
> +		}
> +
> +		meta->fm_nextents = fmh->nextents;
> +
> +		meta->se = kcalloc(meta->fm_nextents, sizeof(*(meta->se)),
> +				   GFP_KERNEL);

[Severity: Medium]
Should this variable-length extent array allocation also include the
__GFP_ACCOUNT flag? It could consume substantial memory per file.

> +		if (!meta->se)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < fmh->nextents; i++) {
> +			meta->se[i].dev_index  = se_in[i].se_devindex;
> +			meta->se[i].ext_offset = se_in[i].se_offset;
> +			meta->se[i].ext_len    = se_in[i].se_len;
> +
> +			/* Record bitmap of referenced daxdev indices */
> +			meta->dev_bitmap |= BIT_ULL(meta->se[i].dev_index);

[Severity: Medium]
Could this trigger a shift-out-of-bounds if the FUSE server provides a
dev_index >= 64? There doesn't appear to be a bounds check on
se_in[i].se_devindex prior to this bitwise shift operation.

[ ... ]
> +	/* More sanity checks */
> +	if (extent_total < meta->file_size) {
> +		pr_err("%s: file size %ld larger than map size %ld\n",
> +		       __func__, meta->file_size, extent_total);
> +		return -EINVAL;
> +	}

[Severity: High]
Can this comparison bypass validation on 32-bit architectures? In
struct famfs_file_meta, file_size is defined as a size_t, which is 32 bits
on those systems. The FUSE wire format provides it as a 64-bit integer,
so any file size > 4GB could be silently truncated. Would it be better
to declare file_size as a 64-bit type like loff_t or u64?

> +
> +	if (cmpxchg(metap, NULL, meta) != NULL) {
> +		pr_debug("%s: fmap race detected\n", __func__);
> +		return 0; /* fmap already installed */
> +	}

[Severity: Low]
Is this race detection effective here? metap points to the local stack variable
declared as struct famfs_file_meta *meta = NULL; in famfs_file_init_dax().
Because it is an unshared local variable, the cmpxchg will always succeed,
rendering this branch dead code.

[ ... ]
> +int
> +famfs_file_init_dax(
> +	struct fuse_mount *fm,
> +	struct inode *inode,
> +	void *fmap_buf,
> +	size_t fmap_size)
> +{
> +	struct fuse_inode *fi = get_fuse_inode(inode);
> +	struct famfs_file_meta *meta = NULL;
> +	int rc;
> +
> +	if (fi->famfs_meta) {
> +		pr_notice("%s: i_no=%llu fmap_size=%zu ALREADY INITIALIZED\n",
> +			  __func__,
> +			  inode->i_ino, fmap_size);
> +		return 0;
> +	}

[Severity: High]
Can concurrent threads observe an incomplete initialization state here?

If thread A publishes the metadata pointer via famfs_meta_set() but has
not yet fully initialized i_size and S_DAX, thread B could evaluate
fi->famfs_meta as true and immediately return.

Thread B might then proceed to read() and locklessly evaluate IS_DAX(inode)
as false, falling back to the non-DAX page cache path.

[ ... ]
> +	/* Publish the famfs metadata on fi->famfs_meta */
> +	inode_lock(inode);
> +
> +	if (famfs_meta_set(fi, meta) == NULL) {
> +		i_size_write(inode, meta->file_size);
> +		inode->i_flags |= S_DAX;

[Severity: High]
Can this direct bitwise modification of inode->i_flags cause lost updates?

The modification is performed without holding inode->i_lock. Since FUSE
can concurrently modify i_flags in fuse_change_attributes_common() while
holding fi->lock, this could result in a data race.

Should this use inode_set_flags() or set_mask_bits() instead?

> diff --git a/fs/fuse/famfs_kfmap.h b/fs/fuse/famfs_kfmap.h
> index 0000000000000..d87b065e8ac83
> --- /dev/null
> +++ b/fs/fuse/famfs_kfmap.h
[ ... ]
> +struct famfs_file_meta {
> +	bool                   error;
> +	enum famfs_file_type   file_type;
> +	size_t                 file_size;

[Severity: High]
Does this type cause truncation on 32-bit systems? The FUSE wire format
struct fuse_famfs_fmap_header provides file_size as a uint64_t, but size_t
is 32 bits on 32-bit architectures.

> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 5bacc50986203..46a7040b38dc4 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
[ ... ]
> @@ -1364,7 +1367,10 @@ static inline struct fuse_backing *famfs_meta_set(struct fuse_inode *fi,

[Severity: Low]
Does famfs_meta_set() return the wrong type? It is declared to return a
struct fuse_backing *, but cmpxchg(&fi->famfs_meta, NULL, meta) returns a
void *.

>  
>  static inline void famfs_meta_free(struct fuse_inode *fi)
>  {
> -	famfs_meta_set(fi, NULL);
> +	if (fi->famfs_meta != NULL) {
> +		__famfs_meta_free(fi->famfs_meta);
> +		WRITE_ONCE(fi->famfs_meta, NULL);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=4