Re: [PATCH V12 03/12] famfs: Add daxdev table and dax notify_failure support

[sashiko-bot@kernel.org]

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 <John@Groves.net>

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-000000@email.amazonses.com?part=3