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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect memory ordering primitive for publishing lockless state (wmb() vs smp_store_release())
--

commit df7a50134ff93b3213d9bbccb1aca11a71421bee
Author: John Groves <John@Groves.net>

famfs: Add daxdev table and dax notify_failure support

This commit introduces the daxdev table for famfs, which manages multiple dax
devices using a basic structure, primitives, and serialization. It also adds
dax_holder_operations and a notify_failure handler required to properly open
a famfs-mode daxdev.

> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> index 5a13903da61b7..5735d8d1900bd 100644
> --- a/fs/famfs/famfs_inode.c
> +++ b/fs/famfs/famfs_inode.c

[ ... ]

> @@ -76,6 +76,231 @@ famfs_get_inode(
> +int
> +famfs_install_daxdev(
> +	struct famfs_fs_info *fsi,
> +	struct super_block *sb,
> +	u64 index,
> +	dev_t devno,
> +	const char *name)
> +{

[ ... ]

> +		daxdev->devno = devno;
> +		if (name) {
> +			daxdev->name = kstrdup(name, GFP_KERNEL);
> +			if (!daxdev->name) {
> +				fs_put_dax(daxdev->devp, sb);
> +				put_dax(daxdev->devp);
> +				daxdev->devp = NULL;
> +				return -ENOMEM;
> +			}
> +		}
> +
> +		wmb(); /* All other fields must be visible before valid */
> +		daxdev->valid = 1;

[Severity: High]
Does this code safely publish the daxdev structure to concurrent lockless
readers?

In famfs_install_daxdev(), the structure is populated and then its valid flag
is set to 1 after a naked wmb(). While wmb() provides a write barrier for the
writer, it does not enforce pairing on the reader side. If a concurrent
lockless reader (such as famfs_daxdev_from_index() added later in this patch
series) checks valid using a simple read like if (!dd->valid) without a
corresponding read barrier, the CPU could reorder the reads.

If the reads are reordered, the reader could observe valid == 1 but see
uninitialized data or a NULL devp, which might lead to a NULL pointer
dereference.

Could this be converted to use smp_store_release(&daxdev->valid, 1) on the
writer side, and paired with smp_load_acquire() on the lockless reader side?

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000000@email.amazonses.com?part=3