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 <[email protected]> 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-000...@email.amazonses.com?part=7
