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

[sashiko-bot@kernel.org]

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 same function.
--

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

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 >= fsi->dax_devlist->nslots)
> +			return -EINVAL;
> +		if (fsi->dax_devlist->devlist[dd.daxdev_index].valid)
> +			return 0;
> +	}
> +
> +	if (dd.daxdev_path_len == 0 || dd.daxdev_path_len >= PATH_MAX)
> +		return -EINVAL;
> +
> +	/* +1 so the terminating NUL is included within the bound */
> +	path = strndup_user((const char __user *)(uintptr_t)dd.daxdev_path,
> +			    dd.daxdev_path_len + 1);
> +	if (IS_ERR(path))
> +		return PTR_ERR(path);
> +
> +	rc = 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 the
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 = 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;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000000@email.amazonses.com?part=9