Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Integer overflow and undefined behavior when calculating `dev_bitmap` with a 32-bit signed shift.
- [High] Recursive `inode_lock()` deadlock on `O_TRUNC` file open.
- [Medium] Data truncation of file size and mapping size on 32-bit architectures.
- [Medium] Dynamic toggling of `S_DAX` flag introduces a data race and violates VFS caching invariants.
- [Low] Dangling pointer window in `famfs_meta_free` due to incorrect cleanup ordering.
- [Low] Meaningless local `cmpxchg` race check acts as dead code.
- [Low] Uninitialized variable accessed and printed in error message.
- [Low] Implicit cast of untrusted UAPI enum to internal enum.
--
--- Patch [4]: [PATCH V8 04/10] famfs_fuse: Create files with famfs fmaps ---
commit 588d2cb1a397d88983186c80e9fec874ef42a12e
Author: John Groves <[email protected]>
famfs_fuse: Create files with famfs fmaps
This patch introduces the ability to allocate and initialize in-memory famfs
file metadata mapped to DAX memory. It adds structures for resolving file
maps and sets up the initialization path based on FUSE GET_FMAP responses.
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
[ ... ]
> +static int
> +famfs_fuse_meta_alloc(
> + void *fmap_buf,
> + size_t fmap_buf_size,
> + struct famfs_file_meta **metap)
> +{
> + struct fuse_famfs_fmap_header *fmh;
[ ... ]
> + meta->error = false;
> + meta->file_type = fmh->file_type;
> + meta->file_size = fmh->file_size;
[Severity: Medium]
Does this truncate the file size on 32-bit architectures?
The FUSE protocol correctly provides this as a 64-bit value, but the
famfs_file_meta structure declares file_size as a size_t. On 32-bit systems,
size_t is a 32-bit unsigned integer.
Any file size larger than 4GB will be silently truncated when assigned here,
which will later result in incorrect sizes passed to i_size_write().
> + meta->fm_extent_type = fmh->ext_type;
[Severity: Low]
Is it safe to directly assign this FUSE UAPI enum to the internal kernel
enum?
The code assigns a UAPI uint32_t (enum famfs_ext_type) directly to a
kernel internal enum famfs_extent_type. While the integer values currently
align identically, this bypasses type safety and will fail silently if the
enumerated types ever diverge.
> +
> + switch (fmh->ext_type) {
> + case FUSE_FAMFS_EXT_SIMPLE: {
> + struct fuse_famfs_simple_ext *se_in;
[ ... ]
> + 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 |= (1 << meta->se[i].dev_index);
[Severity: High]
Can this shift operation invoke undefined behavior or corrupt the bitmap?
The FUSE server provides a dev_index which is used to set a bit in a 64-bit
bitmap. Because 1 is a signed 32-bit integer literal, a dev_index of 31
results in a negative value that sign-extends to 0xFFFFFFFF80000000 when
promoted to u64, incorrectly setting bits 32-63.
If dev_index is 32 or greater, the shift invokes undefined behavior. Using
1ULL would prevent this.
> +
> + errs += famfs_check_ext_alignment(&meta->se[i]);
> +
> + extent_total += meta->se[i].ext_len;
> + }
> + break;
> + }
> +
> + case FUSE_FAMFS_EXT_INTERLEAVE: {
[ ... ]
> + for (i = 0; i < niext; i++) {
> + u64 nstrips;
> + struct fuse_famfs_simple_ext *sie_in;
> +
> + /* ie_in = one interleaved extent in fmap_buf */
> + ie_in = fmap_buf + next_offset;
> +
> + /* Move past one interleaved extent header in fmap_buf */
> + next_offset += sizeof(*ie_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;
> + }
> +
> + if (!IS_ALIGNED(ie_in->ie_chunk_size, PMD_SIZE)) {
> + pr_err("%s: chunk_size %lld not PMD-aligned\n",
> + __func__, meta->ie[i].fie_chunk_size);
[Severity: Low]
Does this error message print an uninitialized value?
During the interleaved extent alignment checks, if ie_chunk_size is not
PMD-aligned, the error message attempts to log meta->ie[i].fie_chunk_size.
However, this field has not yet been assigned from the FUSE payload. The
assignment happens several lines below this check, so this will print 0
instead of the actual invalid chunk size.
> + return -EINVAL;
> + }
[ ... ]
> + /* Inner loop is over strips */
> + for (j = 0; j < nstrips; j++) {
> + struct famfs_meta_simple_ext *strips_out;
> + u64 devindex = sie_in[j].se_devindex;
> + u64 offset = sie_in[j].se_offset;
> + u64 len = sie_in[j].se_len;
> +
> + strips_out = meta->ie[i].ie_strips;
> + strips_out[j].dev_index = devindex;
> + strips_out[j].ext_offset = offset;
> + strips_out[j].ext_len = len;
> +
> + /* Record bitmap of referenced daxdev indices */
> + meta->dev_bitmap |= (1 << devindex);
[Severity: High]
As above, can this shift operation invoke undefined behavior or corrupt the
bitmap if devindex is 32 or greater?
> +
> + extent_total += len;
> + errs += famfs_check_ext_alignment(&strips_out[j]);
> + size_remainder -= len;
> + }
> + }
[ ... ]
> + if (cmpxchg(metap, NULL, meta) != NULL) {
> + pr_debug("%s: fmap race detected\n", __func__);
> + return 0; /* fmap already installed */
> + }
[Severity: Low]
Does this cmpxchg check accomplish its intended race detection?
The metap argument points to a purely local stack variable inside the caller
famfs_file_init_dax(). Since this variable is completely local to the
thread, the cmpxchg will always succeed.
This makes the race detection dead code. The actual functional race check
correctly occurs later via famfs_meta_set().
> + retain_and_null_ptr(meta);
> +
> + return 0;
> +}
> +
> +/**
> + * famfs_file_init_dax() - init famfs dax file metadata
[ ... ]
> +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;
[ ... ]
> + rc = famfs_fuse_meta_alloc(fmap_buf, fmap_size, &meta);
> + if (rc)
> + goto errout;
> +
> + /* Publish the famfs metadata on fi->famfs_meta */
> + inode_lock(inode);
[Severity: High]
Could this unconditional lock acquisition cause an A-A self-deadlock?
When a file is opened with O_TRUNC and writeback caching is enabled, the
fuse_open() path acquires the inode_lock to perform truncation.
If the filesystem is mounted with famfs iomap support, the open path calls
fuse_get_fmap(), which in turn calls famfs_file_init_dax().
This function unconditionally calls inode_lock() again to install the
metadata. Since the same thread already holds the lock, it will deadlock.
> +
> + if (famfs_meta_set(fi, meta) == NULL) {
> + i_size_write(inode, meta->file_size);
> + inode->i_flags |= S_DAX;
[Severity: Medium]
Is it safe to dynamically toggle the DAX flag on an active inode?
This modifies the flags using a read-modify-write without the standard
inode_set_flags() helper.
More importantly, changing S_DAX after an inode has been instantiated
breaks VFS invariants. If page cache pages were populated prior to this
flag change, subsequent I/O will use DAX paths while stale page cache pages
remain, potentially triggering VM_BUG_ON or silent data corruption.
> + } else {
> + pr_debug("%s: file already had metadata\n", __func__);
> + __famfs_meta_free(meta);
> + /* rc is 0 - the file is valid */
> + }
> +
> + inode_unlock(inode);
> + return 0;
[ ... ]
> --- /dev/null
> +++ b/fs/fuse/famfs_kfmap.h
> @@ -0,0 +1,67 @@
[ ... ]
> +/*
> + * Each famfs dax file has this hanging from its fuse_inode->famfs_meta
> + */
> +struct famfs_file_meta {
> + bool error;
> + enum famfs_file_type file_type;
> + size_t file_size;
[Severity: Medium]
Should this be a u64 or loff_t?
As mentioned earlier, declaring file_size as a size_t on 32-bit systems
limits it to a 32-bit unsigned integer, causing truncation for files
larger than 4GB.
> + enum famfs_extent_type fm_extent_type;
> + u64 dev_bitmap; /* bitmap of referenced daxdevs by index */
[ ... ]
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1642,6 +1642,9 @@ extern void fuse_sysctl_unregister(void);
> /* famfs.c */
>
> #if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
[ ... ]
> 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);
> + famfs_meta_set(fi, NULL);
> + }
> }
[Severity: Low]
Does this create a dangling pointer window?
The metadata memory is freed via __famfs_meta_free() before the pointer
is nullified by famfs_meta_set().
While this occurs during inode eviction where concurrency is limited, it
creates a brief use-after-free window and breaks the expected safety
guarantees of a cleanup helper. Reversing the order would be safer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.