Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Race condition in `famfs_dax_notify_failure` causes NULL pointer dereference or Use-After-Free during teardown. - [Medium] famfs_install_daxdev silently returns success when a conflicting device is installed at an already-populated index. --
--- Patch [3]: [PATCH V12 03/12] famfs: Add daxdev table and dax notify_failure support --- commit 6ec60105b0cea4058f8c078b72261124109aca1d Author: John Groves <[email protected]> famfs: Add daxdev table and dax notify_failure support Famfs file systems can span multiple dax devices, and daxdevs are stored in the daxdev_table. This adds the basic table structure, primtives and serialization code. Famfs file extents reference daxdevs by index, which is a cluster invariant maintained by user space. diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c index c299a90912a5c..ad71e5e7a8e39 100644 --- a/fs/famfs/famfs_inode.c +++ b/fs/famfs/famfs_inode.c @@ -75,6 +75,225 @@ static struct inode *famfs_get_inode( /* * famfs dax_operations (for famfs-mode dax) */ [ ... ] > +static int > +famfs_dax_notify_failure( > + struct dax_device *dax_dev, u64 offset, > + u64 len, int mf_flags) > +{ > + struct super_block *sb = dax_holder(dax_dev); > + struct famfs_fs_info *fsi = sb->s_fs_info; [Severity: High] Can this race with a concurrent filesystem unmount? If a memory failure triggers famfs_dax_notify_failure() concurrently with famfs_kill_sb(), fs_put_dax() may clear the holder data asynchronously. If dax_holder() returns NULL here, does this result in a NULL pointer dereference when accessing sb->s_fs_info? Furthermore, if dax_holder() fetches the superblock just before it is cleared: CPU1 famfs_dax_notify_failure() sb = dax_holder(dax_dev); fsi = sb->s_fs_info; CPU2 famfs_kill_sb() famfs_devlist_free(fsi, sb); kill_char_super(sb); kfree(fsi); CPU1 famfs_set_daxdev_err(fsi, dax_dev); Could this result in a use-after-free when famfs_set_daxdev_err() attempts to acquire the freed fsi->devlist_sem? [ ... ] > +int famfs_install_daxdev( > + struct famfs_fs_info *fsi, > + struct super_block *sb, > + u64 index, > + dev_t devno, > + const char *name) > +{ [ ... ] > + scoped_guard(rwsem_write, &fsi->devlist_sem) { > + daxdev = &fsi->dax_devlist->devlist[index]; > + > + /* Installed already by a concurrent (or repeated) open */ > + if (daxdev->valid) > + return 0; [Severity: Medium] If userspace erroneously requests mapping a new or different dax device to an index that is already populated, this path silently returns 0. Should this verify that the requested devno matches the already installed device's devno, and return an error like -EBUSY or -EEXIST if it does not, to prevent misleading userspace about which mapping is actually active? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000...@email.amazonses.com?part=3
