Re: [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure

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

On 5/30/26 9:50 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Clear dev_dax->pgmap on probe failure for dynamic devices. After the dynamic
> path sets dev_dax->pgmap, if a later probe step fails, devres frees the
> devm_kzalloc'd pgmap but leaves dev_dax->pgmap dangling.  Subsequent probe
> attempts would hit the "dynamic-dax with pre-populated page map" check and fail
> permanently. Use a goto cleanup to NULL dev_dax->pgmap on error.
> 
> Fixes: d5406bd458b0a ("dax: add fsdev.c driver for fs-dax on character dax")
> Signed-off-by: John Groves <john@groves.net>
> ---
>  drivers/dax/fsdev.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index dbd722ed7ab05..42aac7e952516 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -223,6 +223,7 @@ 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;
> +	bool pgmap_allocated = false;
>  	struct dev_pagemap *pgmap;
>  	struct inode *inode;
>  	u64 data_offset = 0;
> @@ -253,6 +254,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  
>  		pgmap->nr_range = dev_dax->nr_range;
>  		dev_dax->pgmap = pgmap;
> +		pgmap_allocated = true;
>  
>  		for (i = 0; i < dev_dax->nr_range; i++) {
>  			struct range *range = &dev_dax->ranges[i].range;
> @@ -268,7 +270,8 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  					range_len(range), dev_name(dev))) {
>  			dev_warn(dev, "mapping%d: %#llx-%#llx could not reserve range\n",
>  				 i, range->start, range->end);
> -			return -EBUSY;
> +			rc = -EBUSY;
> +			goto err_pgmap;
>  		}
>  	}
>  
> @@ -288,8 +291,10 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	pgmap->owner = dev_dax;
>  
>  	addr = devm_memremap_pages(dev, pgmap);
> -	if (IS_ERR(addr))
> -		return PTR_ERR(addr);
> +	if (IS_ERR(addr)) {
> +		rc = PTR_ERR(addr);
> +		goto err_pgmap;
> +	}
>  
>  	/*
>  	 * Clear any stale compound folio state left over from a previous
> @@ -301,7 +306,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	rc = devm_add_action_or_reset(dev, fsdev_clear_folio_state_action,
>  				      dev_dax);
>  	if (rc)
> -		return rc;
> +		goto err_pgmap;
>  
>  	/* Detect whether the data is at a non-zero offset into the memory */
>  	if (pgmap->range.start != dev_dax->ranges[0].range.start) {
> @@ -323,23 +328,32 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  	cdev_set_parent(cdev, &dev->kobj);
>  	rc = cdev_add(cdev, dev->devt, 1);
>  	if (rc)
> -		return rc;
> +		goto err_pgmap;
>  
>  	rc = devm_add_action_or_reset(dev, fsdev_cdev_del, cdev);
>  	if (rc)
> -		return rc;
> +		goto err_pgmap;
>  
>  	/* Set the dax operations for fs-dax access path */
>  	rc = dax_set_ops(dax_dev, &dev_dax_ops);
>  	if (rc)
> -		return rc;
> +		goto err_pgmap;
>  
>  	rc = devm_add_action_or_reset(dev, fsdev_clear_ops, dev_dax);
>  	if (rc)
> -		return rc;
> +		goto err_pgmap;
>  
>  	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)
> +		goto err_pgmap;
> +
> +	return 0;
> +
> +err_pgmap:
> +	if (pgmap_allocated)
> +		dev_dax->pgmap = NULL;
> +	return rc;
>  }
>  
>  static struct dax_device_driver fsdev_dax_driver = {


How about something like this to ditch the gotos?

---

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index dbd722ed7ab0..cb309847e685 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -219,6 +219,41 @@ static const struct file_operations fsdev_fops = {
 	.release = fsdev_release,
 };
 
+static struct dev_pagemap *fsdev_acquire_pgmap(struct dev_dax *dev_dax)
+{
+	struct device *dev = &dev_dax->dev;
+	struct dev_pagemap *pgmap;
+	size_t pgmap_size;
+
+	if (static_dev_dax(dev_dax)) {
+		if (dev_dax->nr_range > 1) {
+			dev_warn(dev,
+				 "static vs multi-range device conflict\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		pgmap = dev_dax->pgmap;
+		pgmap->vmemmap_shift = 0;
+		return pgmap;
+	}
+
+	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 ERR_PTR(-ENOMEM);
+
+	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;
+
+	return pgmap;
+}
+
 static int fsdev_dax_probe(struct dev_dax *dev_dax)
 {
 	struct dax_device *dax_dev = dev_dax->dax_dev;
@@ -230,36 +265,9 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
 	void *addr;
 	int rc, i;
 
-	if (static_dev_dax(dev_dax)) {
-		if (dev_dax->nr_range > 1) {
-			dev_warn(dev, "static pgmap / multi-range device conflict\n");
-			return -EINVAL;
-		}
-
-		pgmap = dev_dax->pgmap;
-		pgmap->vmemmap_shift = 0;
-	} else {
-		size_t pgmap_size;
-
-		if (dev_dax->pgmap) {
-			dev_warn(dev, "dynamic-dax with pre-populated page map\n");
-			return -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->nr_range = dev_dax->nr_range;
-		dev_dax->pgmap = pgmap;
-
-		for (i = 0; i < dev_dax->nr_range; i++) {
-			struct range *range = &dev_dax->ranges[i].range;
-
-			pgmap->ranges[i] = *range;
-		}
-	}
+	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 +314,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 +347,12 @@ 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;
+
+	dev_dax->pgmap = pgmap;
+	return 0;
 }
 
 static struct dax_device_driver fsdev_dax_driver = {