Re: [PATCH V2 6/7] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[John Groves]

On 26/05/26 05:28PM, Dave Jiang wrote:
> 
> 
> On 5/22/26 12:19 PM, John Groves wrote:
> > From: John Groves <John@xxxxxxxxxx>
> > 
> > This fix is in response to a Sashiko review, and some subsequent
> > analysis.
> > 
> > dax_dev_get() uses iget5_locked() which creates a new inode if no
> > matching one exists. This is correct for the internal caller
> > (alloc_dax), but dangerous for external callers that look up devices
> > from user-supplied or metadata-supplied dev_t values:
> > 
> > 1. A new inode is created with DAXDEV_ALIVE set but no backing driver,
> >    no ops, and no IDA-allocated minor number.
> > 
> > 2. On teardown, dax_destroy_inode() warns because kill_dax() was never
> >    called, and dax_free_inode() calls ida_free() for a minor that was
> >    never ida_alloc'd — potentially freeing the minor of a real device.
> > 
> > Add dax_dev_find() which uses ilookup5() for lookup-only semantics:
> > it returns an existing dax_device with an elevated inode reference, or
> > NULL if no device with the given dev_t exists. It never creates 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.
> > 
> > Fixes: 2ae624d5a555d ("dax: export dax_dev_get()")
> > Signed-off-by: John Groves <john@xxxxxxxxxx>
> > ---
> >  drivers/dax/super.c | 27 +++++++++++++++++++++++++--
> >  include/linux/dax.h |  6 +++++-
> >  2 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> > index fa1d2a6eb2408..79e5823d1010d 100644
> > --- a/drivers/dax/super.c
> > +++ b/drivers/dax/super.c
> > @@ -541,7 +541,7 @@ static int dax_set(struct inode *inode, void *data)
> >  	return 0;
> >  }
> >  
> > -struct dax_device *dax_dev_get(dev_t devt)
> > +static struct dax_device *dax_dev_get(dev_t devt)
> >  {
> >  	struct dax_device *dax_dev;
> >  	struct inode *inode;
> > @@ -564,7 +564,30 @@ 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 inode *inode;
> > +
> > +	inode = ilookup5(dax_superblock, hash_32(devt + DAXFS_MAGIC, 31),
> > +			 dax_test, &devt);
> > +	if (!inode)
> > +		return NULL;
> > +
> 
> Claude mentions that dax_alive() check may be a good idea after grabbing the inode ref. Otherwise famfs may get a dax_dev that may be racing with a teardown. Do something similar that fs_dax_get_by_dev() or fs_dax_get() do WRT dax_alive() check perhaps.
> 
> DJ

I think that's right. Calling dax_alive() requires a dax_read_lock();
Adding that too.

Thanks!
John

<snip>