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

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 d=
ereference 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_fai=
lure 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 =3D dax_holder(dax_dev);
> +	struct famfs_fs_info *fsi =3D 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 derefere=
nce
when accessing sb->s_fs_info?

Furthermore, if dax_holder() fetches the superblock just before it is clear=
ed:

CPU1
famfs_dax_notify_failure()
    sb =3D dax_holder(dax_dev);
    fsi =3D 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 =3D &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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/0100019fc572ca94-ec=
[email protected]?part=3D3