Re: [PATCH V4 4/9] dax/fsdev: don't leave a dangling dev_dax->pgmap on probe failure

[Dave Jiang <dave.jiang@intel.com>]

On 6/7/26 12:33 PM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> After the dynamic path set dev_dax->pgmap, any later probe failure left
> dev_dax->pgmap dangling: devres frees the devm_kzalloc'd pgmap on probe
> failure, and subsequent probe attempts would hit the "dynamic-dax with
> pre-populated page map" check and fail permanently.
> 
> Factor pgmap acquisition out into fsdev_acquire_pgmap(), and defer the
> dev_dax->pgmap assignment until probe can no longer fail. A failed probe
> now never publishes the pointer at all, so there is nothing to unwind.
> This also matches kill_dev_dax(), which already clears the dynamic pgmap
> pointer on unbind: dev_dax->pgmap is now non-NULL only while the pgmap
> is actually valid.
> 
> Refactor suggested by Dave Jiang.
> 
> Fixes: d5406bd458b0a ("dax: add fsdev.c driver for fs-dax on character dax")
> Signed-off-by: John Groves <john@groves.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/dax/fsdev.c | 77 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index dbd722ed7ab05..0fd5e1293d725 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -219,47 +219,62 @@ static const struct file_operations fsdev_fops = {
>  	.release = fsdev_release,
>  };
>  
> -static int fsdev_dax_probe(struct dev_dax *dev_dax)
> +/*
> + * Acquire the dev_pagemap for probe: the static (pre-populated) one if
> + * present, or a devm-allocated one for the dynamic case. Note that
> + * dev_dax->pgmap is not set here; fsdev_dax_probe() sets it only once
> + * probe succeeds, so a failed probe never leaves a dangling pointer
> + * to a devres-freed pgmap.
> + */
> +static struct dev_pagemap *fsdev_acquire_pgmap(struct dev_dax *dev_dax)
>  {
> -	struct dax_device *dax_dev = dev_dax->dax_dev;
>  	struct device *dev = &dev_dax->dev;
>  	struct dev_pagemap *pgmap;
> -	struct inode *inode;
> -	u64 data_offset = 0;
> -	struct cdev *cdev;
> -	void *addr;
> -	int rc, i;
> +	size_t pgmap_size;
>  
>  	if (static_dev_dax(dev_dax)) {
>  		if (dev_dax->nr_range > 1) {
> -			dev_warn(dev, "static pgmap / multi-range device conflict\n");
> -			return -EINVAL;
> +			dev_warn(dev,
> +				 "static pgmap / multi-range device conflict\n");
> +			return ERR_PTR(-EINVAL);
>  		}
>  
>  		pgmap = dev_dax->pgmap;
>  		pgmap->vmemmap_shift = 0;
> -	} else {
> -		size_t pgmap_size;
> +		return pgmap;
> +	}
>  
> -		if (dev_dax->pgmap) {
> -			dev_warn(dev, "dynamic-dax with pre-populated page map\n");
> -			return -EINVAL;
> -		}
> +	if (dev_dax->pgmap) {
> +		dev_warn(dev, "dynamic-dax with pre-populated page map\n");
> +		return ERR_PTR(-EINVAL);
> +	}
>  
> -		pgmap_size = struct_size(pgmap, ranges, dev_dax->nr_range - 1);
> -		pgmap = devm_kzalloc(dev, pgmap_size, GFP_KERNEL);
> -		if (!pgmap)
> -			return -ENOMEM;
> +	pgmap_size = struct_size(pgmap, ranges, dev_dax->nr_range - 1);
> +	pgmap = devm_kzalloc(dev, pgmap_size, GFP_KERNEL);
> +	if (!pgmap)
> +		return ERR_PTR(-ENOMEM);
>  
> -		pgmap->nr_range = dev_dax->nr_range;
> -		dev_dax->pgmap = pgmap;
> +	pgmap->nr_range = dev_dax->nr_range;
> +	for (int i = 0; i < dev_dax->nr_range; i++)
> +		pgmap->ranges[i] = dev_dax->ranges[i].range;
>  
> -		for (i = 0; i < dev_dax->nr_range; i++) {
> -			struct range *range = &dev_dax->ranges[i].range;
> +	return pgmap;
> +}
>  
> -			pgmap->ranges[i] = *range;
> -		}
> -	}
> +static int fsdev_dax_probe(struct dev_dax *dev_dax)
> +{
> +	struct dax_device *dax_dev = dev_dax->dax_dev;
> +	struct device *dev = &dev_dax->dev;
> +	struct dev_pagemap *pgmap;
> +	struct inode *inode;
> +	u64 data_offset = 0;
> +	struct cdev *cdev;
> +	void *addr;
> +	int rc, i;
> +
> +	pgmap = fsdev_acquire_pgmap(dev_dax);
> +	if (IS_ERR(pgmap))
> +		return PTR_ERR(pgmap);
>  
>  	for (i = 0; i < dev_dax->nr_range; i++) {
>  		struct range *range = &dev_dax->ranges[i].range;
> @@ -306,7 +321,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	/* Detect whether the data is at a non-zero offset into the memory */
>  	if (pgmap->range.start != dev_dax->ranges[0].range.start) {
>  		u64 phys = dev_dax->ranges[0].range.start;
> -		u64 pgmap_phys = dev_dax->pgmap[0].range.start;
> +		u64 pgmap_phys = pgmap[0].range.start;
>  
>  		if (!WARN_ON(pgmap_phys > phys))
>  			data_offset = phys - pgmap_phys;
> @@ -339,7 +354,13 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		return rc;
>  
>  	run_dax(dax_dev);
> -	return devm_add_action_or_reset(dev, fsdev_kill, dev_dax);
> +	rc = devm_add_action_or_reset(dev, fsdev_kill, dev_dax);
> +	if (rc)
> +		return rc;
> +
> +	/* Probe can no longer fail; expose the pgmap via dev_dax */
> +	dev_dax->pgmap = pgmap;
> +	return 0;
>  }
>  
>  static struct dax_device_driver fsdev_dax_driver = {