Re: [PATCH v3 1/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dax: partially initialized dax_device returned by dax_dev_find() due to premature unlock_new_inode()
--

commit 9b2ac5a45a1d5eca0b23e84e793ce64303f44b4e
Author: John Groves <John@Groves.net>

dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

dax_dev_get() uses iget5_locked() which creates a new inode if no
matching one exists, which is dangerous for external callers. This patch
introduces dax_dev_find() to provide lookup-only semantics without allocating
new inodes, and restricts dax_dev_get() to internal allocation use.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 45f84b0eb909a..824e1f6df3780 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c

[ ... ]

> @@ -588,7 +588,41 @@ struct dax_device *dax_dev_get(dev_t devt)
>  
>  	return dax_dev;
>  }
> -EXPORT_SYMBOL_GPL(dax_dev_get);
> +
> +/**
> + * dax_dev_find - look up an existing dax_device by dev_t
> + * @devt: the device number to find
> + *
> + * Returns a dax_device with an elevated inode reference, or NULL if no
> + * device with the given dev_t exists. Unlike dax_dev_get(), this never
> + * allocates a new inode -- it is safe for external callers that are looking
> + * up devices from user-supplied or metadata-supplied dev_t values.
> + *
> + * Caller must put_dax() the returned device when done.
> + */
> +struct dax_device *dax_dev_find(dev_t devt)
> +{
> +	struct dax_device *dax_dev;
> +	struct inode *inode;
> +	int id;
> +
> +	inode = ilookup5(dax_superblock, hash_32(devt + DAXFS_MAGIC, 31),
> +			 dax_test, &devt);
> +	if (!inode)
> +		return NULL;
> +
> +	dax_dev = to_dax_dev(inode);
> +	id = dax_read_lock();
> +	if (!dax_alive(dax_dev)) {
        ^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this introduce a regression where a concurrent external caller of
dax_dev_find() can obtain a partially initialized device?

When a thread calls alloc_dax(), it calls dax_dev_get() to allocate the inode.
Inside dax_dev_get(), the new inode is instantiated, DAXDEV_ALIVE is set, and
unlock_new_inode() is called:

drivers/dax/super.c:dax_dev_get() {
    ...
    if (inode_state_read_once(inode) & I_NEW) {
        set_bit(DAXDEV_ALIVE, &dax_dev->flags);
        ...
        unlock_new_inode(inode);
    }
    return dax_dev;
}

This clears the I_NEW flag, making the inode fully accessible to concurrent
VFS lookups before dax_dev_get() returns.

If a concurrent thread calls dax_dev_find() at this moment, ilookup5() will
find the unlocked inode and return it. Since DAXDEV_ALIVE is already set,
dax_dev_find() will successfully return the device.

However, the allocating thread has not yet initialized the operations and
private data in alloc_dax():

drivers/dax/super.c:alloc_dax() {
    ...
    dax_dev = dax_dev_get(devt);
    if (!dax_dev)
        goto err_dev;

    dax_dev->ops = ops;
    dax_dev->private = private;
    return dax_dev;
}

Will this sequence allow an external caller to access a dax_device with
uninitialized ops and private pointers?

> +		dax_read_unlock(id);
> +		iput(inode);
> +		return NULL;
> +	}
> +	dax_read_unlock(id);
> +
> +	return dax_dev;
> +}
> +EXPORT_SYMBOL_GPL(dax_dev_find);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006180115.1425232-1-mszeredi@redhat.com?part=1