Re: [PATCH V14 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:
- [High] Lockless read of daxdev->valid lacks a read memory barrier, leading to a TOCTOU race that can yield a NULL pointer and a zero error code.
--

commit 592a1b20aa272f01a78424966dd5d63af6c63cf8
Author: John Groves <John@Groves.net>

famfs: Register secondary daxdevs by path (FAMFSIOC_DAXDEV_OPEN)

Famfs file maps (fmaps) may reference multiple daxdevs. Before passing
an fmap that references a new daxdev, the daxdev is pushed into the
kernel via FAMFSIOC_DAXDEV_OPEN). This adds daxdevs to daxdev_table for
index-based resolution from famfs extents to daxdevs.

> kernel via FAMFSIOC_DAXDEV_OPEN). This adds daxdevs to daxdev_table for

This isn't a bug, but there is an unmatched closing parenthesis after
FAMFSIOC_DAXDEV_OPEN.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> index 2200f168aa79d..eba80dab0562d 100644
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c
[ ... ]
> +static int
> +famfs_daxdev_open(struct file *file, void __user *arg)
> +{
[ ... ]
> +	/*
> +	 * 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);

[Severity: High]
Does exposing famfs_install_daxdev() to concurrent execution via this ioctl
trigger a race condition in the lockless readers?

famfs_install_daxdev() uses wmb() to ensure daxdev->devp is visible before
setting daxdev->valid = 1:

fs/famfs/famfs_inode.c:famfs_install_daxdev() {
    ...
    wmb(); /* All other fields must be visible before valid */
    daxdev->valid = 1;
    ...
}

However, the readers famfs_dax_err() and famfs_daxdev_from_index() lack a
pairing smp_rmb():

fs/famfs/famfs_file.c:famfs_dax_err() {
    if (!dd->valid) {
        pr_debug("%s: daxdev=%s invalid\n", __func__, dd->name);
        return -EIO;
    }
    ...
}

fs/famfs/famfs_file.c:famfs_daxdev_from_index() {
    ...
    rc = famfs_dax_err(dd);
    if (rc) {
        *errp = rc;
        return NULL;
    }
    *errp = 0;
    return dd->devp;
}

Could the CPU or compiler reorder the loads, reading a stale NULL for devp
before reading valid == 1, resulting in famfs_daxdev_from_index() returning
NULL with a success error code (0)?

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