Re: [PATCH V11 9/9] famfs_fuse: Add documentation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The DAX notify_failure callback dereferences a NULL or freed fuse_conn pointer due to a lack of synchronization with fs_put_dax().
- [Medium] Casting a u64 pointer to unsigned long * for for_each_set_bit() breaks on 32-bit big-endian architectures.
--

--- Patch [9]: [PATCH V11 9/9] famfs_fuse: Add documentation ---
commit ad461f8692da789ab34a9eaac4a65cb121d91c61
Author: John Groves <[email protected]>

famfs_fuse: Add documentation

This patch series implements the famfs dax file system, which provides a
mechanism for multiple hosts to concurrently access data in shared memory via
a fuse interface. It adds memory error handling via dax_holder_operations,
plumbs dax iomap operations, and introduces the required fuse messages. This
commit specifically adds the documentation and MAINTAINERS entries.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> new file mode 100644
> index 0000000000000..aea0bceef774f
> --- /dev/null
> +++ b/fs/fuse/famfs.c
> @@ -0,0 +1,1066 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * famfs - dax file system for shared fabric-attached memory

[ ... ]

> +static int
> +famfs_dax_notify_failure(struct dax_device *dax_devp, u64 offset,
> +			u64 len, int mf_flags)
> +{
> +	struct fuse_conn *fc = dax_holder(dax_devp);
> +
> +	famfs_set_daxdev_err(fc, dax_devp);

[Severity: High]
Can this sequence cause a NULL pointer dereference or use-after-free?

If fs_put_dax() clears dax_dev->holder_data to NULL without waiting for SRCU
readers, and a concurrent memory failure notification runs
dax_holder_notify_failure(), dax_holder() could return NULL here.

Additionally, even if a valid pointer is read before it's cleared, couldn't
fuse_conn_put() free the fuse_conn via call_rcu() without waiting for the
SRCU grace period used by dax_holder_notify_failure()?

> +
> +	return 0;
> +}

[ ... ]

> +static int
> +famfs_check_daxdev_table(
> +	struct fuse_mount *fm,
> +	const struct famfs_file_meta *meta)
> +{
> +	struct fuse_conn *fc = fm->fc;
> +	int nmissing = 0;
> +	int err;
> +
> +	err = famfs_devlist_alloc(fc);
> +	if (err)
> +		return err;
> +
> +	/* Count missing daxdevs while holding the reader lock */
> +	scoped_guard(rwsem_read, &fc->famfs_devlist_sem) {
> +		unsigned long i;
> +
> +		for_each_set_bit(i, (unsigned long *)&meta->dev_bitmap,
> +				 MAX_DAXDEVS) {

[Severity: Medium]
Does this cast break on 32-bit big-endian architectures?

When casting a u64 pointer to an unsigned long pointer on these architectures,
it points to the high 32 bits of the u64. Since MAX_DAXDEVS is 24, all valid
bits are stored in the low 32 bits.

Won't the loop scan the all-zero high 32 bits and completely skip the required
daxdev validation for the file?

> +			struct famfs_daxdev *dd = &fc->dax_devlist->devlist[i];
> +
> +			/*
> +			 * Skip daxdevs already installed (valid) or already
> +			 * known to be unusable (dax_err). Re-fetching either
> +			 * just thrashes on GET_DAXDEV and fs_dax_get().
> +			 */
> +			if (!dd->valid && !dd->dax_err)
> +				nmissing++;
> +		}
> +	}

[ ... ]

> +static void
> +famfs_set_daxdev_err(
> +	struct fuse_conn *fc,
> +	struct dax_device *dax_devp)
> +{
> +	int i;
> +
> +	/*
> +	 * Search the list by dax_devp under the write lock: we set dd->error,
> +	 * and it serializes against famfs_teardown() clearing the table.
> +	 */
> +	scoped_guard(rwsem_write, &fc->famfs_devlist_sem) {

[Severity: High]
Is a NULL check for fc needed here? As mentioned above, if fc is passed in as
NULL from famfs_dax_notify_failure(), this will immediately dereference it.

> +		if (!fc->dax_devlist)
> +			return;
> +		for (i = 0; i < fc->dax_devlist->nslots; i++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=9