Re: [PATCH V12 09/12] famfs: Register secondary daxdevs by path (FAMFSIOC_DAXDEV_OPEN)

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:
- [Low] Mixing goto-based cleanup with scope-based cleanup helpers in the s=
ame function.
--

--- Patch [9]: [PATCH V12 09/12] famfs: Register secondary daxdevs by path =
(FAMFSIOC_DAXDEV_OPEN) ---
commit 92fac6008c0bb8bb7ad31e3ae10289f93997e74b
Author: John Groves <[email protected]>

famfs: Register secondary daxdevs by path (FAMFSIOC_DAXDEV_OPEN)

This commit introduces the FAMFSIOC_DAXDEV_OPEN ioctl to register devdax
devices by path into the famfs mount's daxdev table. This allows files with
extents referencing these secondary devices to be mapped.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c

[ ... ]

> +static int
> +famfs_daxdev_open(struct file *file, void __user *arg)
> +{

[ ... ]

> +	scoped_guard(rwsem_read, &fsi->devlist_sem) {
> +		if (dd.daxdev_index >=3D fsi->dax_devlist->nslots)
> +			return -EINVAL;
> +		if (fsi->dax_devlist->devlist[dd.daxdev_index].valid)
> +			return 0;
> +	}
> +
> +	if (dd.daxdev_path_len =3D=3D 0 || dd.daxdev_path_len >=3D PATH_MAX)
> +		return -EINVAL;
> +
> +	/* +1 so the terminating NUL is included within the bound */
> +	path =3D strndup_user((const char __user *)(uintptr_t)dd.daxdev_path,
> +			    dd.daxdev_path_len + 1);
> +	if (IS_ERR(path))
> +		return PTR_ERR(path);
> +
> +	rc =3D lookup_daxdev(path, &devno);
> +	if (rc)
> +		goto out;

[Severity: Low]
Does famfs_daxdev_open() mix goto-based cleanup with scope-based cleanup
helpers?

This function uses scoped_guard() earlier, but relies on a traditional goto
out label for manual memory cleanup of the path variable. The cleanup
subsystem guidelines recommend against mixing goto-based error handling and
scope-based cleanup in the same function to avoid confusing ownership
semantics.

Could path be declared with the __free() helper to eliminate the need for t=
he
goto label entirely?

> +
> +	/*
> +	 * The daxdev table is allocated at mount time (for the slot-0 primary),
> +	 * so it is always present here; no need to allocate it.
> +	 */
> +	rc =3D famfs_install_daxdev(fsi, sb, dd.daxdev_index, devno, path);
> +	if (rc)
> +		pr_debug("%s: failed to install daxdev index %llu (%s)\n",
> +		       __func__, dd.daxdev_index, path);
> +out:
> +	kfree(path);
> +	return rc;
> +}

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