Re: [PATCH V12 01/12] 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:

Pre-existing issues:
- [High] Race Condition / Uninitialized Data Access during DAX device allocation.
--

--- Patch [1]: [PATCH V12 01/12] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() ---
commit ff957b44b15e4c0e4aaedcf9cd2d12118ab1bb27
Author: John Groves <John@Groves.net>

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

This patch introduces dax_dev_find() to look up existing DAX devices
without allocating a new inode, preventing empty inodes from being created
when external callers provide user-supplied device numbers. It also makes
dax_dev_get() static and updates external references.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..0fc14d4f81989 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
[ ... ]
> @@ -544,7 +544,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
[ ... ]
> + */
> +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)) {
> +		dax_read_unlock(id);
> +		iput(inode);
> +		return NULL;
> +	}
> +	dax_read_unlock(id);
> +
> +	return dax_dev;
> +}

[Severity: High]
This isn't a bug introduced by this patch, but could a concurrent caller of
dax_dev_find() obtain a partially initialized device here?

In dax_dev_get(), unlock_new_inode() clears I_NEW and makes the DAX device
visible to ilookup5(). However, alloc_dax() sets the ops and private
pointers after dax_dev_get() returns:

alloc_dax() {
    ...
    dax_dev = dax_dev_get(devt);
    if (!dax_dev)
        goto err_dev;

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

Because dax_dev_find() retrieves the device from the inode cache and only
checks if DAXDEV_ALIVE is set, could an external caller using a guessed
device number observe NULL ops or private pointers? 

If the external caller attempts to use the returned device before alloc_dax()
completes, could this lead to a NULL pointer dereference and kernel panic?

> +EXPORT_SYMBOL_GPL(dax_dev_find);

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