Re: [PATCH V4 8/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 2 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces an exported symbol `dax_dev_find` but does not include the claimed caller updates, resulting in zero in-tree users.

Pre-existing issues:
- [High] A race condition allows `dax_dev_find()` to return a partially initialized `dax_device` with stale or uninitialized `ops` and `private` pointers, leading to a Use-After-Free or NULL pointer dereference.
--

commit 4db396b1477c7ac48536ccf90d47d3c4adb9e2d3
Author: John Groves <John@Groves.net>

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

This commit replaces the exported dax_dev_get with a new dax_dev_find
function that performs lookup-only semantics. It also makes dax_dev_get
static again to prevent external callers from accidentally creating new
unbacked DAX inodes.

> Make dax_dev_get() static again (internal to super.c for alloc_dax),
> export dax_dev_find() instead, and update the two external callers
> (famfs_inode.c, famfs.c). Also add the missing CONFIG_DAX=n stub.

[Severity: Medium]
The commit message mentions updating external callers in famfs_inode.c
and famfs.c, but these files are not included in the patch and do not
exist in the upstream tree.

Is it possible the updates for these files were left out of the patch,
or are they out-of-tree? Exporting a symbol without any in-tree users
typically goes against upstream guidelines.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 96f778dcde50b..b37ae79c084bb 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
[ ... ]
> +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]
This is a pre-existing issue, but does adding dax_dev_find expose a race
condition where a partially initialized device can be returned to external
callers?

When a new DAX device is created, alloc_dax calls dax_dev_get. Inside
dax_dev_get, the new inode is unlocked and DAXDEV_ALIVE is set:

    if (inode_state_read_once(inode) & I_NEW) {
        set_bit(DAXDEV_ALIVE, &dax_dev->flags);
        ...
        unlock_new_inode(inode);
    }

However, alloc_dax initializes the ops and private pointers after
dax_dev_get returns:

    dax_dev = dax_dev_get(devt);
    if (!dax_dev)
        goto err_dev;

    dax_dev->ops = ops;
    dax_dev->private = private;

Since dax_device is allocated via alloc_inode_sb with GFP_KERNEL (no
__GFP_ZERO), could a concurrent call to dax_dev_find successfully look up
the inode via ilookup5 and see DAXDEV_ALIVE set before alloc_dax sets
the ops and private pointers?

If an external caller invokes DAX operations on the returned device, could
it dereference dangling ops or private pointers from a previous slab object
lifecycle? Is it possible to defer unlock_new_inode or setting
DAXDEV_ALIVE until after ops and private are fully initialized?

> +		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/0100019ea3929225-a0f8e6f7-30ae-4f8e-ae6f-19129666c4c3-000000@email.amazonses.com?part=8