[PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series

[PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series

[John Groves]

From: John Groves <john@groves.net>

This series applies bug fixes (mostly found via sashiko) to the dax/fsdev 
series. This has been soaking in the famfs CI pipeline for 2+ weeks and
1) won't affect anything that doesn't use drivers/dax/fsdev.c, and 2)
doesn't affect any known workloads - although the bugs would have 
manifested when multi-range DCD dax devices are a thing (soon-ish).

Changes since V2:

* Patch 1 (comment fix): No change. Responded to Dave's question about
  the dropped precondition -- the new comment correctly covers both
  callers; fsdev_clear_folio_state() does not guarantee share==0 before
  calling, so the old precondition was no longer universally true.
* V2 patch 2 (three fixes): Split into three separate patches (patches
  2-4) per Dave's review.
* V2 patch 3 (two fixes): Split into two separate patches (patches 5-6)
  per Dave's review.
* V2 patch 4 (clamp direct_access / remove cached_size): Dropped.
  Dave's analysis correctly showed the claimed bug does not exist --
  dax_pgoff_to_phys() already enforces that the full requested size fits
  within a single range before returning, making the clamp a no-op in
  every reachable path.
* V2 patch 5 (holder_ops race): Use WRITE_ONCE() for the holder_ops
  store; add WARN_ON() on the cmpxchg result to catch wrong-holder and
  double-put API contract violations; fix the inline comment, which
  incorrectly claimed dax_holder_notify_failure() consults holder_ops
  only when holder_data is non-NULL.
* V2 patch 6 (dax_dev_find): Add dax_alive() check under dax_read_lock()
  after ilookup5() to prevent returning a device that is concurrently
  being torn down by kill_dax().
* V2 patch 7 (formatting cleanup): Drop incorrect Fixes: tag; add
  Dave's Reviewed-by.
* The series grows from 7 to 9 patches.

Changes since v1:
* Dropped modes from patch 6 to fs/fuse/famfs.c and 
  fs/famfs/famfs_inode.c, which are not upstream so it broke
  attempts to apply the series. Oops...
* Added patch 7, which addresses a previously-missed review comment
  from Jonathan - minor cleanup

John Groves (9):
  dax: fix misleading comment about share/index union in
    dax_folio_reset_order()
  dax/fsdev: fix multi-range offset in memory_failure handler
  dax/fsdev: clear vmemmap_shift when binding static pgmap
  dax/fsdev: clear dev_dax->pgmap on probe failure
  dax/fsdev: use __va(phys) for kaddr in direct_access
  dax/fsdev: fail probe on invalid pgmap offset
  dax: fix holder_ops race in fs_put_dax()
  dax: replace exported dax_dev_get() with non-allocating dax_dev_find()
  dax: fsdev.c minor formatting cleanup

 drivers/dax/fsdev.c | 81 +++++++++++++++++++++++++++++++--------------
 drivers/dax/super.c | 73 +++++++++++++++++++++++++++++++++++++---
 fs/dax.c            | 12 +++----
 include/linux/dax.h |  6 +++-
 4 files changed, 136 insertions(+), 36 deletions(-)

-- 
2.53.0

[PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order()

[John Groves]

From: John Groves <John@Groves.net>

The comment in dax_folio_reset_order() claims that DAX maintains an
invariant where folio->share != 0 only when folio->mapping == NULL,
implying folio->share is zero whenever mapping is non-NULL. This is
misleading because folio->share and folio->index are a union -- for
non-shared folios with mapping != NULL, reading folio->share returns
the file page offset (folio->index), which is typically non-zero.

Reword the comment to accurately describe the union aliasing: the
assignment clears whichever interpretation of the union word is active
(index for non-shared folios, share for shared folios), which is correct
because the folio is being released in either case.

No functional change -- the code was already correct, only the
justification was wrong.

Fixes: 59eb73b98ae0b ("dax: Factor out dax_folio_reset_order() helper")

Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: John Groves <john@groves.net>
---
 fs/dax.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 6d175cd47a99b..df19c9317d10e 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -392,12 +392,12 @@ int dax_folio_reset_order(struct folio *folio)
 	int order = folio_order(folio);
 
 	/*
-	 * DAX maintains the invariant that folio->share != 0 only when
-	 * folio->mapping == NULL (enforced by dax_folio_make_shared()).
-	 * Equivalently: folio->mapping != NULL implies folio->share == 0.
-	 * Callers ensure share has been decremented to zero before
-	 * calling here, so unconditionally clearing both fields is
-	 * correct.
+	 * Clear the mapping and the index/share union word. folio->share
+	 * and folio->index occupy the same union in struct folio. For
+	 * non-shared folios (mapping != NULL), the union holds folio->index
+	 * (file page offset); for shared folios (mapping == NULL), it holds
+	 * folio->share (reference count). Either way, we are releasing the
+	 * folio and both fields should be zeroed.
 	 */
 	folio->mapping = NULL;
 	folio->share = 0;
-- 
2.53.0

[PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[John Groves]

From: John Groves <John@Groves.net>

Fix memory_failure offset calculation for multi-range devices. The old code
subtracted ranges[0].range.start from the faulting PFN's physical address,
which produces an incorrect (inflated) logical offset when the PFN falls in
ranges[1] or beyond due to physical gaps between ranges. Add
fsdev_pfn_to_offset() to walk the range list and compute the correct
device-linear byte offset.

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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index 188b2526bee45..f315533b299e9 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -135,11 +135,26 @@ static void fsdev_clear_ops(void *data)
  * The core mm code in free_zone_device_folio() handles the wake_up_var()
  * directly for this memory type.
  */
+static u64 fsdev_pfn_to_offset(struct dev_dax *dev_dax, unsigned long pfn)
+{
+	phys_addr_t phys = PFN_PHYS(pfn);
+	u64 offset = 0;
+
+	for (int i = 0; i < dev_dax->nr_range; i++) {
+		struct range *range = &dev_dax->ranges[i].range;
+
+		if (phys >= range->start && phys <= range->end)
+			return offset + (phys - range->start);
+		offset += range_len(range);
+	}
+	return -1ULL;
+}
+
 static int fsdev_pagemap_memory_failure(struct dev_pagemap *pgmap,
 		unsigned long pfn, unsigned long nr_pages, int mf_flags)
 {
 	struct dev_dax *dev_dax = pgmap->owner;
-	u64 offset = PFN_PHYS(pfn) - dev_dax->ranges[0].range.start;
+	u64 offset = fsdev_pfn_to_offset(dev_dax, pfn);
 	u64 len = nr_pages << PAGE_SHIFT;
 
 	return dax_holder_notify_failure(dev_dax->dax_dev, offset,
-- 
2.53.0

[PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[John Groves]

From: John Groves <John@Groves.net>

Clear pgmap->vmemmap_shift for static DAX devices. When rebinding a static
device from device_dax (which may set vmemmap_shift based on alignment) to
fsdev_dax, the stale vmemmap_shift persists on the shared pgmap. Explicitly
zero it before devm_memremap_pages() so the vmemmap is built for order-0
folios as fsdev requires.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index f315533b299e9..dbd722ed7ab05 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -237,6 +237,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
 		}
 
 		pgmap = dev_dax->pgmap;
+		pgmap->vmemmap_shift = 0;
 	} else {
 		size_t pgmap_size;
 
-- 
2.53.0

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

[John Groves]

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 = {
-- 
2.53.0

[PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access

[John Groves]

From: John Groves <John@Groves.net>

Use __va(phys) instead of virt_addr + linear_offset for the kaddr
return in __fsdev_dax_direct_access(). The previous code added a
device-linear byte offset to virt_addr (which is __va of ranges[0]),
but for multi-range devices with physical gaps between ranges, this
linear arithmetic crosses the gap and produces a wrong kernel virtual
address. Using __va(phys) where phys comes from dax_pgoff_to_phys()
is correct for any range layout because the direct map translates
each physical address independently.

Fixes: 759455848df0b ("dax: Save the kva from memremap")
Signed-off-by: John Groves <john@groves.net>
---
 drivers/dax/fsdev.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index 42aac7e952516..a2d2eb20fb4d0 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -51,9 +51,7 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
 	struct dev_dax *dev_dax = dax_get_private(dax_dev);
 	size_t size = nr_pages << PAGE_SHIFT;
 	size_t offset = pgoff << PAGE_SHIFT;
-	void *virt_addr = dev_dax->virt_addr + offset;
 	phys_addr_t phys;
-	unsigned long local_pfn;
 
 	phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
 	if (phys == -1) {
@@ -63,11 +61,10 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
 	}
 
 	if (kaddr)
-		*kaddr = virt_addr;
+		*kaddr = __va(phys);
 
-	local_pfn = PHYS_PFN(phys);
 	if (pfn)
-		*pfn = local_pfn;
+		*pfn = PHYS_PFN(phys);
 
 	/*
 	 * Use cached_size which was computed at probe time. The size cannot
-- 
2.53.0

[PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset

[John Groves]

From: John Groves <John@Groves.net>

Convert the WARN_ON to a fatal error when pgmap_phys > phys. This
condition means the remapped region starts after the device's data
region, which is an impossible state. Previously the probe continued
with data_offset=0, leaving virt_addr silently misaligned. Now probe
returns -EINVAL with a diagnostic message.

Fixes: 759455848df0b ("dax: Save the kva from memremap")
Signed-off-by: John Groves <john@groves.net>
---
 drivers/dax/fsdev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index a2d2eb20fb4d0..aac0130ab2833 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -310,8 +310,13 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
 		u64 phys = dev_dax->ranges[0].range.start;
 		u64 pgmap_phys = dev_dax->pgmap[0].range.start;
 
-		if (!WARN_ON(pgmap_phys > phys))
-			data_offset = phys - pgmap_phys;
+		if (pgmap_phys > phys) {
+			dev_err(dev, "pgmap start %#llx exceeds data start %#llx\n",
+				pgmap_phys, phys);
+			rc = -EINVAL;
+			goto err_pgmap;
+		}
+		data_offset = phys - pgmap_phys;
 
 		pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx\n",
 		       __func__, phys, pgmap_phys, data_offset);
-- 
2.53.0

[PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax()

[John Groves]

From: John Groves <John@Groves.net>

Clear holder_ops before holder_data so that a concurrent fs_dax_get()
cannot have its newly installed holder_ops overwritten. cmpxchg()
provides release ordering on weakly-ordered architectures, ensuring the
WRITE_ONCE(holder_ops, NULL) store is visible to any CPU that observes
the holder_data release.

Add WARN_ON() on the cmpxchg result to catch two API contract
violations: fs_put_dax() called by a non-holder, or called twice by
the same holder (double-put). Either way holder_ops has already been
cleared, so WARN_ON() does not prevent the damage but makes the bug
visible. (Note: "damage" is only if a non-holder causes holder_ops
to be cleared)

Also add a kerneldoc comment documenting that fs_put_dax() must only
be called by the current holder.

Fixes: eec38f5d86d27 ("dax: Add fs_dax_get() func to prepare dax for fs-dax usage")
Signed-off-by: John Groves <john@groves.net>
---
 drivers/dax/super.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 25cf99dd9360b..4c56ac2faacdb 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -116,11 +116,40 @@ EXPORT_SYMBOL_GPL(fs_dax_get_by_bdev);
 
 #if IS_ENABLED(CONFIG_FS_DAX)
 
+/**
+ * fs_put_dax() - release holder ownership of a dax_device
+ * @dax_dev: dax device to release (may be NULL)
+ * @holder: the holder pointer previously passed to fs_dax_get() or
+ *          fs_dax_get_by_bdev(); must match exactly, as it is used
+ *          in a cmpxchg to atomically release ownership
+ *
+ * Must only be called by the current holder. Clears holder_ops before
+ * holder_data to avoid a race where a concurrent fs_dax_get() could have
+ * its newly installed holder_ops overwritten.
+ */
 void fs_put_dax(struct dax_device *dax_dev, void *holder)
 {
-	if (dax_dev && holder &&
-	    cmpxchg(&dax_dev->holder_data, holder, NULL) == holder)
-		dax_dev->holder_ops = NULL;
+	if (dax_dev && holder) {
+		/*
+		 * Clear holder_ops before releasing holder_data. A concurrent
+		 * dax_holder_notify_failure() that sees NULL ops returns
+		 * -EOPNOTSUPP cleanly. A concurrent fs_dax_get() that acquires
+		 * holder_data after the cmpxchg below is guaranteed to observe
+		 * holder_ops=NULL first (cmpxchg provides release ordering), so
+		 * its subsequent store of new ops will not be overwritten.
+		 *
+		 * Two cases will trigger the WARN_ON():
+		 * - Caller is not the current holder; this is an API contract
+		 *   violation, and the holder will no longer get callbacks
+		 * - Holder calls this function twice; also a contract violation
+		 *
+		 * A lock would be necessary to guard against the contract
+		 * violations, but we WARN_ON() instead since violating the
+		 * contract is a bug
+		 */
+		WRITE_ONCE(dax_dev->holder_ops, NULL);
+		WARN_ON(cmpxchg(&dax_dev->holder_data, holder, NULL) != holder);
+	}
 	put_dax(dax_dev);
 }
 EXPORT_SYMBOL_GPL(fs_put_dax);
-- 
2.53.0

[PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[John Groves]

From: John Groves <John@Groves.net>

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.
A dax_alive() check under dax_read_lock() guards against returning a
device that is concurrently being torn down by kill_dax().

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@groves.net>
---
 drivers/dax/super.c | 38 ++++++++++++++++++++++++++++++++++++--
 include/linux/dax.h |  6 +++++-
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 4c56ac2faacdb..6cb271e034a70 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -550,7 +550,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;
@@ -573,7 +573,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
+ * @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 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;
+}
+EXPORT_SYMBOL_GPL(dax_dev_find);
 
 struct dax_device *alloc_dax(void *private, const struct dax_operations *ops)
 {
diff --git a/include/linux/dax.h b/include/linux/dax.h
index fe6c3ded1b50f..29113eb95e72d 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -54,7 +54,7 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops);
 void *dax_holder(struct dax_device *dax_dev);
 void put_dax(struct dax_device *dax_dev);
 void kill_dax(struct dax_device *dax_dev);
-struct dax_device *dax_dev_get(dev_t devt);
+struct dax_device *dax_dev_find(dev_t devt);
 void dax_write_cache(struct dax_device *dax_dev, bool wc);
 bool dax_write_cache_enabled(struct dax_device *dax_dev);
 bool dax_synchronous(struct dax_device *dax_dev);
@@ -92,6 +92,10 @@ static inline void put_dax(struct dax_device *dax_dev)
 static inline void kill_dax(struct dax_device *dax_dev)
 {
 }
+static inline struct dax_device *dax_dev_find(dev_t devt)
+{
+	return NULL;
+}
 static inline void dax_write_cache(struct dax_device *dax_dev, bool wc)
 {
 }
-- 
2.53.0

[PATCH V3 9/9] dax: fsdev.c minor formatting cleanup

[John Groves]

From: John Groves <John@Groves.net>

Address some comments from Jonathan that were missed in the merged
series. Fix line wrapping in fsdev_dax_recovery_write() and
fsdev_dax_zero_page_range() signatures.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: John Groves <john@groves.net>
---
 drivers/dax/fsdev.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
index aac0130ab2833..ff936021d7aec 100644
--- a/drivers/dax/fsdev.c
+++ b/drivers/dax/fsdev.c
@@ -45,8 +45,8 @@ static void fsdev_write_dax(void *addr, struct page *page,
 }
 
 static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
-			long nr_pages, enum dax_access_mode mode, void **kaddr,
-			unsigned long *pfn)
+		long nr_pages, enum dax_access_mode mode, void **kaddr,
+		unsigned long *pfn)
 {
 	struct dev_dax *dev_dax = dax_get_private(dax_dev);
 	size_t size = nr_pages << PAGE_SHIFT;
@@ -80,7 +80,8 @@ static int fsdev_dax_zero_page_range(struct dax_device *dax_dev,
 	long rc;
 
 	WARN_ONCE(nr_pages > 1, "%s: nr_pages > 1\n", __func__);
-	rc = __fsdev_dax_direct_access(dax_dev, pgoff, 1, DAX_ACCESS, &kaddr, NULL);
+	rc = __fsdev_dax_direct_access(dax_dev, pgoff, 1, DAX_ACCESS,
+				       &kaddr, NULL);
 	if (rc < 0)
 		return rc;
 	fsdev_write_dax(kaddr, ZERO_PAGE(0), 0, PAGE_SIZE);
@@ -88,15 +89,15 @@ static int fsdev_dax_zero_page_range(struct dax_device *dax_dev,
 }
 
 static long fsdev_dax_direct_access(struct dax_device *dax_dev,
-		  pgoff_t pgoff, long nr_pages, enum dax_access_mode mode,
-		  void **kaddr, unsigned long *pfn)
+		pgoff_t pgoff, long nr_pages, enum dax_access_mode mode,
+		void **kaddr, unsigned long *pfn)
 {
 	return __fsdev_dax_direct_access(dax_dev, pgoff, nr_pages, mode,
 					 kaddr, pfn);
 }
 
-static size_t fsdev_dax_recovery_write(struct dax_device *dax_dev, pgoff_t pgoff,
-		void *addr, size_t bytes, struct iov_iter *i)
+static size_t fsdev_dax_recovery_write(struct dax_device *dax_dev,
+		pgoff_t pgoff, void *addr, size_t bytes, struct iov_iter *i)
 {
 	return _copy_from_iter_flushcache(addr, bytes, i);
 }
-- 
2.53.0

Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless iteration over `dev_dax->ranges` in `fsdev_pfn_to_offset` causes a Use-After-Free.
--

commit 104845be5fb1300e103dae52129fee9336fbed14
Author: John Groves <John@Groves.net>
dax/fsdev: fix multi-range offset in memory_failure handler

This commit fixes the memory_failure offset calculation for multi-range
devices by introducing fsdev_pfn_to_offset() to walk the range list and
compute the correct device-linear byte offset.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 188b2526bee45..f315533b299e9 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
[ ... ]
> @@ -135,11 +135,26 @@ static void fsdev_clear_ops(void *data)
>   * The core mm code in free_zone_device_folio() handles the wake_up_var()
>   * directly for this memory type.
>   */
> +static u64 fsdev_pfn_to_offset(struct dev_dax *dev_dax, unsigned long pfn)
> +{
> +	phys_addr_t phys = PFN_PHYS(pfn);
> +	u64 offset = 0;
> +
> +	for (int i = 0; i < dev_dax->nr_range; i++) {
> +		struct range *range = &dev_dax->ranges[i].range;

[Severity: High]
This is a pre-existing issue, but does iterating over dev_dax->ranges here
without locks expose us to a use-after-free?

Because fsdev_pfn_to_offset() is called asynchronously by the memory_failure
handler, it seems possible for it to race with root writing to the mapping
sysfs attribute to append ranges. That write invokes mapping_store().

Unlike size_store(), mapping_store() does not check if the DAX device is
currently bound to a driver before calling alloc_dev_dax_range(). That function
uses krealloc() to expand the dev_dax->ranges array, freeing the old memory.

If fsdev_pfn_to_offset() executes concurrently with this sysfs write, could it
dereference the freed array pointer?

Would it be safer to pass the pgmap parameter down from the memory failure
handler and iterate over pgmap->ranges instead, since it contains a stable,
immutable snapshot of the ranges created during driver probe?

> +
> +		if (phys >= range->start && phys <= range->end)
> +			return offset + (phys - range->start);
> +		offset += range_len(range);
> +	}
> +	return -1ULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=2

Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Dangling pointer vulnerability (Use-After-Free) in pgmap->ops on driver unbind
- [Critical] Invalid Virtual Address Calculation for disjoint DAX ranges causing data corruption
- [High] Probe failure leaves dangling dev_dax->pgmap preventing device rebinding
--

commit fd314d6aab8b188d638ba9782dcab88646401f10
Author: John Groves <John@Groves.net>

dax/fsdev: clear vmemmap_shift when binding static pgmap

This commit clears pgmap->vmemmap_shift for static DAX devices before
calling devm_memremap_pages(). This ensures the vmemmap is built for order-0
folios as required by fsdev, explicitly zeroing any stale state left over
from a previous device_dax binding.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index f315533b299e9..dbd722ed7ab05 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
[ ... ]
> @@ -237,6 +237,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		}
>  
>  		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;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
dangling pointer if the probe fails later.

In fsdev_dax_probe(), dev_dax->pgmap is assigned the devm_kzalloc'd pgmap.
If a subsequent probe step fails:

fsdev_dax_probe()
    dev_dax->pgmap = pgmap;
    ...
    addr = devm_memremap_pages(dev, pgmap);
    if (IS_ERR(addr))
        return PTR_ERR(addr);

The devm framework will free the pgmap memory upon returning the error, but
dev_dax->pgmap is never reset to NULL.

Subsequent attempts to re-bind the driver will see dev_dax->pgmap != NULL
and fail permanently with -EINVAL, erroneously claiming a pre-populated
page map.

Does this need a cleanup action to clear dev_dax->pgmap on failure?

Note that this was fixed later in the series by "dax/fsdev: clear
dev_dax->pgmap on probe failure".

[ ... ]
> 	pgmap->type = MEMORY_DEVICE_FS_DAX;
> 	pgmap->ops = &fsdev_pagemap_ops;
> 	pgmap->owner = dev_dax;
> 
> 	addr = devm_memremap_pages(dev, pgmap);
> 	if (IS_ERR(addr))
> 		return PTR_ERR(addr);

[Severity: Critical]
This is a pre-existing issue, but I am concerned about a dangling pointer
vulnerability on driver unbind.

When binding static DAX devices, fsdev_dax_probe() assigns the module-local
&fsdev_pagemap_ops to pgmap->ops and dev_dax to pgmap->owner.

However, there appears to be no corresponding cleanup action in fsdev_kill()
or fsdev_clear_ops() to clear these fields when the driver is unbound.

If the static device is subsequently bound to device_dax (which reuses the
same pgmap but does not overwrite pgmap->ops), and the dax_fsdev module is
unloaded, pgmap->ops becomes a dangling pointer to freed module memory.

Could a hardware memory failure in the mapped DAX range then trigger
memory_failure_dev_pagemap(), dereferencing the stale
pgmap->ops->memory_failure pointer and causing a kernel panic?

[ ... ]
> 	if (kaddr)
> 		*kaddr = dev_dax->virt_addr + offset;

[Severity: Critical]
This is a pre-existing issue, but the virtual address computation here
seems to assume a contiguous virtual memory layout.

For dynamic DAX devices spanning multiple physically disjoint ranges,
__fsdev_dax_direct_access() calculates the kernel virtual address using
linear arithmetic:

    void *virt_addr = dev_dax->virt_addr + offset;

Because virt_addr maps directly over physical memory via __va(), wouldn't
a physical gap between ranges cause the linearly calculated virt_addr to
point into the unmapped gap rather than the correctly mapped subsequent
physical memory?

Does this return an invalid pointer for offsets beyond the first range,
leading to data corruption when callers read or write memory?

Note that this was fixed later in the series by "dax/fsdev: use __va(phys)
for kaddr in direct_access".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Fix / Permanent Probe Failure in `dev_dax_probe()`
--

commit e06a09a1cbe1a16c2fbebd7c33f2df95d4149ac0
Author: John Groves <John@Groves.net>

    dax/fsdev: clear dev_dax->pgmap on probe failure

Clear dev_dax->pgmap on probe failure for dynamic devices to prevent a dangling
pointer that causes subsequent probe attempts to permanently fail.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -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;

[Severity: High]
This is a pre-existing issue, but does this exact same problem also exist in
drivers/dax/device.c:dev_dax_probe()?

In dev_dax_probe(), a dynamic DAX device allocates pgmap via devm_kzalloc()
and assigns it to dev_dax->pgmap. If a subsequent initialization step fails
(e.g., devm_request_mem_region() returning -EBUSY), the function returns
directly without clearing dev_dax->pgmap.

Devres then frees the devm_kzalloc'd pgmap, leaving dev_dax->pgmap as a
dangling pointer.

When the user subsequently attempts to re-bind the driver to the device,
dev_dax_probe() executes "if (dev_dax->pgmap)", sees the non-NULL dangling
pointer, and unconditionally fails with -EINVAL ("dynamic-dax with
pre-populated page map").

This permanently breaks driver binding for the device until the device itself
is completely destroyed. Could dev_dax_probe() in device.c use a similar fix?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=4

Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch leaves behind dead code for calculating `data_offset` and assigning `dev_dax->virt_addr` in `fsdev_dax_probe()`.

Pre-existing issues:
- [High] `__fsdev_dax_direct_access()` returns `-EFAULT` when the requested `size` crosses a physical range boundary, instead of truncating the return value to the available contiguous pages.
--

commit b771878942b230ec4df1330dccd98b1983cf30b8
Author: John Groves <John@Groves.net>

dax/fsdev: use __va(phys) for kaddr in direct_access

This commit fixes the kernel virtual address calculation in
__fsdev_dax_direct_access() by using __va(phys) instead of a linear
offset from virt_addr. This ensures correct behavior for multi-range
devices with physical gaps.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 42aac7e952516..a2d2eb20fb4d0 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -51,9 +51,7 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>  	struct dev_dax *dev_dax = dax_get_private(dax_dev);
>  	size_t size = nr_pages << PAGE_SHIFT;
>  	size_t offset = pgoff << PAGE_SHIFT;
> -	void *virt_addr = dev_dax->virt_addr + offset;

[Severity: Low]
By removing this calculation, is virt_addr now entirely unused?

Looking at the rest of the driver, dev_dax->virt_addr doesn't appear to be
read anywhere else. Should the virt_addr field in struct dev_dax and its
assignment in fsdev_dax_probe() be removed to clean up the dead code?

>  	phys_addr_t phys;
> -	unsigned long local_pfn;
>  
>  	phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
>  	if (phys == -1) {
>  		dev_dbg(&dev_dax->dev,
>  			"pgoff (%#lx) out of range\n", pgoff);
>  		return -EFAULT;
>  	}

[Severity: High]
This is a pre-existing issue, but does returning -EFAULT here break the
direct_access API contract when a request crosses a physical range boundary?

If the requested size extends beyond the current physical range on a
multi-range device, dax_pgoff_to_phys() returns -1, causing this to fail
entirely. Should this instead truncate the return value to the available
physically contiguous pages rather than failing the whole transfer?

Additionally, the fallback size calculation later in this function:

return PHYS_PFN(min(size, dev_dax->cached_size - offset));

seems to incorrectly assume physical contiguity across range gaps.

>  
>  	if (kaddr)
> -		*kaddr = virt_addr;
> +		*kaddr = __va(phys);
>  
> -	local_pfn = PHYS_PFN(phys);
>  	if (pfn)
> -		*pfn = local_pfn;
> +		*pfn = PHYS_PFN(phys);
>  
>  	/*
>  	 * Use cached_size which was computed at probe time. The size cannot

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=5

Re: [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Late Validation / Out-of-Bounds Memory Access
--

commit a9f4bec62b4b3e100b1ed11c3e4fa29bdbf57a95
Author: John Groves <John@Groves.net>

dax/fsdev: fail probe on invalid pgmap offset

This commit converts the WARN_ON when pgmap_phys > phys into a fatal error,
failing the probe with -EINVAL. This prevents the probe from continuing with
a misaligned virt_addr when the remapped region starts after the data region.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index a2d2eb20fb4d0..aac0130ab2833 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -310,8 +310,13 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		u64 phys = dev_dax->ranges[0].range.start;
>  		u64 pgmap_phys = dev_dax->pgmap[0].range.start;
>  
> -		if (!WARN_ON(pgmap_phys > phys))
> -			data_offset = phys - pgmap_phys;
> +		if (pgmap_phys > phys) {
> +			dev_err(dev, "pgmap start %#llx exceeds data start %#llx\n",
> +				pgmap_phys, phys);
> +			rc = -EINVAL;
> +			goto err_pgmap;
> +		}

[Severity: High]
This is a pre-existing issue, but does adding this explicit validation check
here occur too late to prevent a kernel panic?

Looking at fsdev_dax_probe(), devm_memremap_pages() populates the vmemmap
starting at pgmap_phys. Just before this block, the code unconditionally
calls fsdev_clear_folio_state(), which iterates over dev_dax->ranges
starting at phys.

If pgmap_phys > phys (the invalid state checked here), phys falls before
the region where the vmemmap is populated. Accessing unpopulated memory
during the folio state cleanup would panic before we even reach this new
validation check.

Additionally, the devres cleanup action is registered immediately before
this check. If this new check fails and jumps to err_pgmap, the devres
unrolling will execute on the error path:

fsdev_dax_probe()
   fsdev_clear_folio_state(dev_dax);
   devm_add_action_or_reset(..., fsdev_clear_folio_state_action, ...);

   if (pgmap_phys > phys) {
       ...
       goto err_pgmap;
   }

Would this trigger the unsafe memory access again during cleanup?

Should the validation of pgmap_phys vs phys be moved earlier in the probe
sequence, before clearing the folio state?

> +		data_offset = phys - pgmap_phys;
>  
>  		pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx\n",
>  		       __func__, phys, pgmap_phys, data_offset);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=6

Re: [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The `WARN_ON` in `fs_put_dax()` will falsely trigger during normal filesystem unmount if the DAX device was hot-unplugged.
- [High] Unconditional clear of `holder_ops` in `fs_put_dax()` before validating ownership can corrupt the state of concurrent valid holders.

Pre-existing issues:
- [High] Lockless double-fetch of `dax_dev->holder_ops` in `dax_holder_notify_failure()` can lead to a NULL pointer dereference.
- [High] Missing SRCU synchronization in `fs_put_dax()` allows use-after-free and module use-after-unload during unmount.
- [High] Inode reference leak in `fs_dax_get_by_bdev()` when a concurrent holder already owns the DAX device.
--

commit 182e9ccdc90a15375d69ff8ae0ad8f75bda03aed
Author: John Groves <John@Groves.net>

dax: fix holder_ops race in fs_put_dax()

Clear holder_ops before holder_data so that a concurrent fs_dax_get()
cannot have its newly installed holder_ops overwritten.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..4c56ac2faacdb 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c

[ ... ]

>  void fs_put_dax(struct dax_device *dax_dev, void *holder)
>  {
> -	if (dax_dev && holder &&
> -	    cmpxchg(&dax_dev->holder_data, holder, NULL) == holder)
> -		dax_dev->holder_ops = NULL;
> +	if (dax_dev && holder) {

[ ... ]

> +		WRITE_ONCE(dax_dev->holder_ops, NULL);
[Severity: High]
Could this unconditional write corrupt the state of concurrent valid holders?

By clearing dax_dev->holder_ops unconditionally before checking ownership
with cmpxchg(), a caller passing the wrong holder could inadvertently
overwrite the holder_ops installed by a legitimate subsystem that just
acquired the DAX device.

> +		WARN_ON(cmpxchg(&dax_dev->holder_data, holder, NULL) != holder);
[Severity: High]
Will this WARN_ON falsely trigger during a normal filesystem unmount if the
DAX device was hot-unplugged?

When a DAX device is hot-unplugged, the driver calls kill_dax(), which sets
dax_dev->holder_data = NULL.

If the user subsequently unmounts the filesystem, fs_put_dax() is called, but
the cmpxchg will see NULL instead of the expected holder. This would cause
the WARN_ON to fire and potentially panic systems configured with
panic_on_warn during normal administrative operations.

> +	}
>  	put_dax(dax_dev);
[Severity: High]
This isn't a bug introduced by this patch, but does fs_put_dax() need SRCU
synchronization before returning?

dax_holder_notify_failure() runs asynchronously under the dax_read_lock()
SRCU critical section. Since fs_put_dax() clears the holder registration but
returns immediately without calling synchronize_srcu(&dax_srcu), could a
filesystem unmount proceed, destroying in-memory structures and unloading
the module while the notify_failure callback is still actively executing?

>  }

[Severity: High]
This isn't a bug introduced by this patch, but does fs_dax_get_by_bdev() leak
an inode reference when a concurrent holder already owns the DAX device?

drivers/dax/super.c:fs_dax_get_by_bdev() {
    ...
	else if (holder) {
		if (!cmpxchg(&dax_dev->holder_data, NULL, holder))
			dax_dev->holder_ops = ops;
		else
			dax_dev = NULL;
	}
    ...
}

When a dax_dev is found, its inode reference count is incremented via
igrab(&dax_dev->inode). If cmpxchg() fails because the device is already
held, dax_dev is set to NULL, but the function appears to return without
calling iput(&dax_dev->inode) to release the reference.

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential lockless
double-fetch of dax_dev->holder_ops in dax_holder_notify_failure() that could
lead to a NULL pointer dereference?

drivers/dax/super.c:dax_holder_notify_failure() {
    ...
	if (!dax_dev->holder_ops) {
		rc = -EOPNOTSUPP;
		goto out;
	}

	rc = dax_dev->holder_ops->notify_failure(dax_dev, off, len, mf_flags);
    ...
}

Since dax_dev->holder_ops is checked for NULL and then immediately
dereferenced without READ_ONCE(), the compiler might load the value twice.
If a concurrent fs_put_dax() executes WRITE_ONCE(dax_dev->holder_ops, NULL)
between these two loads, the second load could fetch NULL and cause a crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=7

Re: [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Recycled `dax_device` objects inherit stale flags and operations due to lack of zero-initialization.
- [High] `alloc_dax()` exposes a partially initialized `dax_device` to concurrent lookups before `ops` and `private` are configured.
- [High] `dax_core_init()` incorrectly returns 0 on failure, leading to use-after-free panics during delayed DAX device allocations.
--

commit d84f23ffdf147bc18ecb76242e383acc9eb6cbc0
Author: John Groves <John@Groves.net>

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

This commit introduces dax_dev_find() for lookup-only semantics and makes
dax_dev_get() static. It resolves teardown warnings and potential minor
number corruption caused by allocating inodes without backing drivers.

> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 4c56ac2faacdb..6cb271e034a70 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
[ ... ]
> @@ -573,7 +573,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)
> +{
[ ... ]
> +	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;
> +}
> +EXPORT_SYMBOL_GPL(dax_dev_find);
>  
>  struct dax_device *alloc_dax(void *private, const struct dax_operations *ops)
>  {
>  	struct dax_device *dax_dev;
[ ... ]
>  	dax_dev = dax_dev_get(devt);
>  	if (!dax_dev)
>  		goto err_dev;
>  
>  	dax_dev->ops = ops;
>  	dax_dev->private = private;

[Severity: High]
This is a pre-existing issue, but does setting the operations pointer here
create a race condition with concurrent lookups?

Looking at dax_dev_get(), it calls unlock_new_inode() which makes the
inode globally visible in the VFS cache before alloc_dax() configures
these critical fields:

drivers/dax/super.c:dax_dev_get() {
    ...
    if (inode_state_read_once(inode) & I_NEW) {
        set_bit(DAXDEV_ALIVE, &dax_dev->flags);
        ...
        unlock_new_inode(inode);
    }
    return dax_dev;
}

Could a concurrent thread calling the newly introduced dax_dev_find()
observe dax_alive() returning true, but dereference an uninitialized or 
stale ops pointer when performing DAX operations?

>  	return dax_dev;
>  
>   err_dev:
[ ... ]


While reviewing this patch, I also noticed a couple of other pre-existing 
issues in drivers/dax/super.c that might be worth investigating:

[Severity: High]
This is a pre-existing issue, but can recycled dax_device objects inherit
stale flags?

In dax_alloc_inode(), the dax_device is allocated from the slab without
clearing the outer fields:

drivers/dax/super.c:dax_alloc_inode() {
    ...
    dax_dev = alloc_inode_sb(sb, dax_cache, GFP_KERNEL);
    ...
}

The init_once() function only clears the memory on the initial slab creation:

drivers/dax/super.c:init_once() {
    ...
    memset(dax_dev, 0, sizeof(*dax_dev));
    ...
}

Since bits like DAXDEV_SYNC and DAXDEV_WRITE_CACHE are set but never cleared
on teardown, could a newly allocated DAX device erroneously inherit these
states and skip required CPU cache flushes?


[Severity: High]
This is a pre-existing issue, but does dax_core_init() hide initialization
failures?

If subsystem initialization fails, the error path destroys critical global
structures but explicitly returns 0:

drivers/dax/super.c:dax_core_init() {
    ...
err_chrdev:
    dax_fs_exit();
    return 0;
}

Would this hide the failure from the kernel's init system, causing delayed
DAX device allocations via alloc_dax() or dax_dev_find() to attempt 
accessing the destroyed dax_cache or unmounted dax_superblock and panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=8

Re: [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order()

[Dave Jiang]

On 5/30/26 9:50 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> The comment in dax_folio_reset_order() claims that DAX maintains an
> invariant where folio->share != 0 only when folio->mapping == NULL,
> implying folio->share is zero whenever mapping is non-NULL. This is
> misleading because folio->share and folio->index are a union -- for
> non-shared folios with mapping != NULL, reading folio->share returns
> the file page offset (folio->index), which is typically non-zero.
> 
> Reword the comment to accurately describe the union aliasing: the
> assignment clears whichever interpretation of the union word is active
> (index for non-shared folios, share for shared folios), which is correct
> because the folio is being released in either case.
> 
> No functional change -- the code was already correct, only the
> justification was wrong.
> 
> Fixes: 59eb73b98ae0b ("dax: Factor out dax_folio_reset_order() helper")
> 
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: John Groves <john@groves.net>

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

> ---
>  fs/dax.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 6d175cd47a99b..df19c9317d10e 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -392,12 +392,12 @@ int dax_folio_reset_order(struct folio *folio)
>  	int order = folio_order(folio);
>  
>  	/*
> -	 * DAX maintains the invariant that folio->share != 0 only when
> -	 * folio->mapping == NULL (enforced by dax_folio_make_shared()).
> -	 * Equivalently: folio->mapping != NULL implies folio->share == 0.
> -	 * Callers ensure share has been decremented to zero before
> -	 * calling here, so unconditionally clearing both fields is
> -	 * correct.
> +	 * Clear the mapping and the index/share union word. folio->share
> +	 * and folio->index occupy the same union in struct folio. For
> +	 * non-shared folios (mapping != NULL), the union holds folio->index
> +	 * (file page offset); for shared folios (mapping == NULL), it holds
> +	 * folio->share (reference count). Either way, we are releasing the
> +	 * folio and both fields should be zeroed.
>  	 */
>  	folio->mapping = NULL;
>  	folio->share = 0;

Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[Dave Jiang]

On 5/30/26 9:50 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Clear pgmap->vmemmap_shift for static DAX devices. When rebinding a static
> device from device_dax (which may set vmemmap_shift based on alignment) to
> fsdev_dax, the stale vmemmap_shift persists on the shared pgmap. Explicitly
> zero it before devm_memremap_pages() so the vmemmap is built for order-0
> folios as fsdev requires.
> 
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index f315533b299e9..dbd722ed7ab05 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -237,6 +237,7 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		}
>  
>  		pgmap = dev_dax->pgmap;
> +		pgmap->vmemmap_shift = 0;
>  	} else {
>  		size_t pgmap_size;
>  

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

[Dave Jiang]

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 = {

Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access

[Dave Jiang]

On 5/30/26 9:51 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Use __va(phys) instead of virt_addr + linear_offset for the kaddr
> return in __fsdev_dax_direct_access(). The previous code added a
> device-linear byte offset to virt_addr (which is __va of ranges[0]),
> but for multi-range devices with physical gaps between ranges, this
> linear arithmetic crosses the gap and produces a wrong kernel virtual
> address. Using __va(phys) where phys comes from dax_pgoff_to_phys()
> is correct for any range layout because the direct map translates
> each physical address independently.
> 
> Fixes: 759455848df0b ("dax: Save the kva from memremap")
> Signed-off-by: John Groves <john@groves.net>

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

one comment below

> ---
>  drivers/dax/fsdev.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 42aac7e952516..a2d2eb20fb4d0 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -51,9 +51,7 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>  	struct dev_dax *dev_dax = dax_get_private(dax_dev);
>  	size_t size = nr_pages << PAGE_SHIFT;
>  	size_t offset = pgoff << PAGE_SHIFT;
> -	void *virt_addr = dev_dax->virt_addr + offset;

With this change, there's no more dev_dax->virt_addr usage? Should that be removed?

DJ

>  	phys_addr_t phys;
> -	unsigned long local_pfn;
>  
>  	phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
>  	if (phys == -1) {
> @@ -63,11 +61,10 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>  	}
>  
>  	if (kaddr)
> -		*kaddr = virt_addr;
> +		*kaddr = __va(phys);
>  
> -	local_pfn = PHYS_PFN(phys);
>  	if (pfn)
> -		*pfn = local_pfn;
> +		*pfn = PHYS_PFN(phys);
>  
>  	/*
>  	 * Use cached_size which was computed at probe time. The size cannot

Re: [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax()

[Dave Jiang]

On 5/30/26 9:51 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Clear holder_ops before holder_data so that a concurrent fs_dax_get()
> cannot have its newly installed holder_ops overwritten. cmpxchg()
> provides release ordering on weakly-ordered architectures, ensuring the
> WRITE_ONCE(holder_ops, NULL) store is visible to any CPU that observes
> the holder_data release.
> 
> Add WARN_ON() on the cmpxchg result to catch two API contract
> violations: fs_put_dax() called by a non-holder, or called twice by
> the same holder (double-put). Either way holder_ops has already been
> cleared, so WARN_ON() does not prevent the damage but makes the bug
> visible. (Note: "damage" is only if a non-holder causes holder_ops
> to be cleared)
> 
> Also add a kerneldoc comment documenting that fs_put_dax() must only
> be called by the current holder.
> 
> Fixes: eec38f5d86d27 ("dax: Add fs_dax_get() func to prepare dax for fs-dax usage")
> Signed-off-by: John Groves <john@groves.net>
> ---
>  drivers/dax/super.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 25cf99dd9360b..4c56ac2faacdb 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -116,11 +116,40 @@ EXPORT_SYMBOL_GPL(fs_dax_get_by_bdev);
>  
>  #if IS_ENABLED(CONFIG_FS_DAX)
>  
> +/**
> + * fs_put_dax() - release holder ownership of a dax_device
> + * @dax_dev: dax device to release (may be NULL)
> + * @holder: the holder pointer previously passed to fs_dax_get() or
> + *          fs_dax_get_by_bdev(); must match exactly, as it is used
> + *          in a cmpxchg to atomically release ownership
> + *
> + * Must only be called by the current holder. Clears holder_ops before
> + * holder_data to avoid a race where a concurrent fs_dax_get() could have
> + * its newly installed holder_ops overwritten.
> + */
>  void fs_put_dax(struct dax_device *dax_dev, void *holder)
>  {
> -	if (dax_dev && holder &&
> -	    cmpxchg(&dax_dev->holder_data, holder, NULL) == holder)
> -		dax_dev->holder_ops = NULL;
> +	if (dax_dev && holder) {
> +		/*
> +		 * Clear holder_ops before releasing holder_data. A concurrent
> +		 * dax_holder_notify_failure() that sees NULL ops returns
> +		 * -EOPNOTSUPP cleanly. A concurrent fs_dax_get() that acquires
> +		 * holder_data after the cmpxchg below is guaranteed to observe
> +		 * holder_ops=NULL first (cmpxchg provides release ordering), so
> +		 * its subsequent store of new ops will not be overwritten.
> +		 *
> +		 * Two cases will trigger the WARN_ON():
> +		 * - Caller is not the current holder; this is an API contract
> +		 *   violation, and the holder will no longer get callbacks
> +		 * - Holder calls this function twice; also a contract violation
> +		 *
> +		 * A lock would be necessary to guard against the contract
> +		 * violations, but we WARN_ON() instead since violating the
> +		 * contract is a bug
> +		 */
> +		WRITE_ONCE(dax_dev->holder_ops, NULL);
> +		WARN_ON(cmpxchg(&dax_dev->holder_data, holder, NULL) != holder);
> +	}
>  	put_dax(dax_dev);
>  }
>  EXPORT_SYMBOL_GPL(fs_put_dax);


This is what Claude Opus 4.8 said:

  The added WARN_ON(cmpxchg(...) != holder) fires on the supported
  device-removal-while-mounted path. kill_dax() (super.c:457) clears holder_data
  = NULL while a holder is still attached — it explicitly tests holder_data !=
  NULL to deliver MF_MEM_PRE_REMOVE first. For xfs on pmem:

  1. pmem_remove() → kill_dax() → MF_MEM_PRE_REMOVE →
  xfs_force_shutdown(SHUTDOWN_FORCE_UMOUNT); the handler does not call
  fs_put_dax. kill_dax then clears holder_data.
  2. Forced unmount → xfs_free_buftarg() → fs_put_dax(bt_daxdev, mp).
  3. cmpxchg(&holder_data, mp, NULL) returns NULL (already cleared) != mp → WARN
  fires, despite xfs being the legitimate holder doing a single put.

  The old == holder form skipped silently in this case. On panic_on_warn systems
  this turns a supported device removal into a panic.

  The commit message's claim that the WARN catches only "non-holder" or
  "double-put" contract violations is incomplete — it also catches the holder
  racing with kill_dax(), which is not a contract violation.

This is the suggested fix:
  void fs_put_dax(struct dax_device *dax_dev, void *holder)
  {
        if (dax_dev && holder) {
                void *prev;

                /*
                 * Clear holder_ops before releasing holder_data so a
                 * concurrent fs_dax_get() that wins holder_data observes
                 * holder_ops == NULL and its store is not overwritten.
                 */
                WRITE_ONCE(dax_dev->holder_ops, NULL);
                prev = cmpxchg(&dax_dev->holder_data, holder, NULL);

                /*
                 * prev == holder: normal release.
                 * prev == NULL:   already released by kill_dax() when the
                 *                 device was removed under a live holder;
                 *                 not a bug.
                 * prev != holder (non-NULL): fs_put_dax() called by something
                 *                 that is not the current holder.
                 */
                WARN_ON(prev && prev != holder);
        }
        put_dax(dax_dev);
  }

Re: [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[Dave Jiang]

On 5/30/26 9:51 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> 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.
> A dax_alive() check under dax_read_lock() guards against returning a
> device that is concurrently being torn down by kill_dax().
> 
> 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()")

Not sure about the Fixes tag when the caller isn't in tree?

> Signed-off-by: John Groves <john@groves.net>

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


> ---
>  drivers/dax/super.c | 38 ++++++++++++++++++++++++++++++++++++--
>  include/linux/dax.h |  6 +++++-
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index 4c56ac2faacdb..6cb271e034a70 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -550,7 +550,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;
> @@ -573,7 +573,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
> + * @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 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;
> +}
> +EXPORT_SYMBOL_GPL(dax_dev_find);
>  
>  struct dax_device *alloc_dax(void *private, const struct dax_operations *ops)
>  {
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index fe6c3ded1b50f..29113eb95e72d 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -54,7 +54,7 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops);
>  void *dax_holder(struct dax_device *dax_dev);
>  void put_dax(struct dax_device *dax_dev);
>  void kill_dax(struct dax_device *dax_dev);
> -struct dax_device *dax_dev_get(dev_t devt);
> +struct dax_device *dax_dev_find(dev_t devt);
>  void dax_write_cache(struct dax_device *dax_dev, bool wc);
>  bool dax_write_cache_enabled(struct dax_device *dax_dev);
>  bool dax_synchronous(struct dax_device *dax_dev);
> @@ -92,6 +92,10 @@ static inline void put_dax(struct dax_device *dax_dev)
>  static inline void kill_dax(struct dax_device *dax_dev)
>  {
>  }
> +static inline struct dax_device *dax_dev_find(dev_t devt)
> +{
> +	return NULL;
> +}
>  static inline void dax_write_cache(struct dax_device *dax_dev, bool wc)
>  {
>  }

Re: [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series

[Alison Schofield]

On Sat, May 30, 2026 at 04:50:02PM +0000, John Groves wrote:
> From: John Groves <john@groves.net>
> 
> This series applies bug fixes (mostly found via sashiko) to the dax/fsdev 
> series. This has been soaking in the famfs CI pipeline for 2+ weeks and
> 1) won't affect anything that doesn't use drivers/dax/fsdev.c, and 2)
> doesn't affect any known workloads - although the bugs would have 
> manifested when multi-range DCD dax devices are a thing (soon-ish).

Hi John,

I'm here to give my Reviewed-by: on all but Patch 7. I'll come back
around after you rev for the Patch 7 feedback from DaveJ. I did a
DaveJ vs Sashiko comparison. (guessing you may have already done that)
DaveJ came out "righter", with a crisper description, suggested fix,
and omits the overstated first issue Sashiko made about the unconditional
write corrupting state.

And since patch 7 shows this set does affect XFS, please update the
above cover letter intro to something like:

"Most of the series is confined to drivers/dax/fsdev.c. Two patches touch
shared DAX core: patch 7 changes fs_put_dax() in drivers/dax/super.c (used
by ext2/ext4/erofs/xfs, though only holder-passing callers, like XFS in-tree,
see a behavior change), and patch 8 adjusts the dax_dev lookup API in
super.c / dax.h."

FWIW I'm overwhelmed with the Sashiko pre-existing defect reports.
I am hoping that patch authors can read thru those and will call out
anything that MUST be fixed for their set, or appear otherwise urgent.

-- Alison


> 
> Changes since V2:
> 
> * Patch 1 (comment fix): No change. Responded to Dave's question about
>   the dropped precondition -- the new comment correctly covers both
>   callers; fsdev_clear_folio_state() does not guarantee share==0 before
>   calling, so the old precondition was no longer universally true.
> * V2 patch 2 (three fixes): Split into three separate patches (patches
>   2-4) per Dave's review.
> * V2 patch 3 (two fixes): Split into two separate patches (patches 5-6)
>   per Dave's review.
> * V2 patch 4 (clamp direct_access / remove cached_size): Dropped.
>   Dave's analysis correctly showed the claimed bug does not exist --
>   dax_pgoff_to_phys() already enforces that the full requested size fits
>   within a single range before returning, making the clamp a no-op in
>   every reachable path.
> * V2 patch 5 (holder_ops race): Use WRITE_ONCE() for the holder_ops
>   store; add WARN_ON() on the cmpxchg result to catch wrong-holder and
>   double-put API contract violations; fix the inline comment, which
>   incorrectly claimed dax_holder_notify_failure() consults holder_ops
>   only when holder_data is non-NULL.
> * V2 patch 6 (dax_dev_find): Add dax_alive() check under dax_read_lock()
>   after ilookup5() to prevent returning a device that is concurrently
>   being torn down by kill_dax().
> * V2 patch 7 (formatting cleanup): Drop incorrect Fixes: tag; add
>   Dave's Reviewed-by.
> * The series grows from 7 to 9 patches.
> 
> Changes since v1:
> * Dropped modes from patch 6 to fs/fuse/famfs.c and 
>   fs/famfs/famfs_inode.c, which are not upstream so it broke
>   attempts to apply the series. Oops...
> * Added patch 7, which addresses a previously-missed review comment
>   from Jonathan - minor cleanup
> 
> John Groves (9):
>   dax: fix misleading comment about share/index union in
>     dax_folio_reset_order()
>   dax/fsdev: fix multi-range offset in memory_failure handler
>   dax/fsdev: clear vmemmap_shift when binding static pgmap
>   dax/fsdev: clear dev_dax->pgmap on probe failure
>   dax/fsdev: use __va(phys) for kaddr in direct_access
>   dax/fsdev: fail probe on invalid pgmap offset
>   dax: fix holder_ops race in fs_put_dax()
>   dax: replace exported dax_dev_get() with non-allocating dax_dev_find()
>   dax: fsdev.c minor formatting cleanup
> 
>  drivers/dax/fsdev.c | 81 +++++++++++++++++++++++++++++++--------------
>  drivers/dax/super.c | 73 +++++++++++++++++++++++++++++++++++++---
>  fs/dax.c            | 12 +++----
>  include/linux/dax.h |  6 +++-
>  4 files changed, 136 insertions(+), 36 deletions(-)
> 
> -- 
> 2.53.0
> 

Re: [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order()

[Alison Schofield]

On Sat, May 30, 2026 at 04:50:35PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> The comment in dax_folio_reset_order() claims that DAX maintains an
> invariant where folio->share != 0 only when folio->mapping == NULL,
> implying folio->share is zero whenever mapping is non-NULL. This is
> misleading because folio->share and folio->index are a union -- for
> non-shared folios with mapping != NULL, reading folio->share returns
> the file page offset (folio->index), which is typically non-zero.
> 
> Reword the comment to accurately describe the union aliasing: the
> assignment clears whichever interpretation of the union word is active
> (index for non-shared folios, share for shared folios), which is correct
> because the folio is being released in either case.
> 
> No functional change -- the code was already correct, only the
> justification was wrong.
> 
> Fixes: 59eb73b98ae0b ("dax: Factor out dax_folio_reset_order() helper")
> 
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: John Groves <john@groves.net>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[Alison Schofield]

On Sat, May 30, 2026 at 04:50:42PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Fix memory_failure offset calculation for multi-range devices. The old code
> subtracted ranges[0].range.start from the faulting PFN's physical address,
> which produces an incorrect (inflated) logical offset when the PFN falls in
> ranges[1] or beyond due to physical gaps between ranges. Add
> fsdev_pfn_to_offset() to walk the range list and compute the correct
> device-linear byte offset.
> 
> Fixes: d5406bd458b0a ("dax: add fsdev.c driver for fs-dax on character dax")
> Signed-off-by: John Groves <john@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap

[Alison Schofield]

On Sat, May 30, 2026 at 04:50:50PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Clear pgmap->vmemmap_shift for static DAX devices. When rebinding a static
> device from device_dax (which may set vmemmap_shift based on alignment) to
> fsdev_dax, the stale vmemmap_shift persists on the shared pgmap. Explicitly
> zero it before devm_memremap_pages() so the vmemmap is built for order-0
> folios as fsdev requires.
> 
> Fixes: d5406bd458b0a ("dax: add fsdev.c driver for fs-dax on character dax")
> Signed-off-by: John Groves <john@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

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

[Alison Schofield]

On Sat, May 30, 2026 at 04:50:57PM +0000, 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>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access

[Alison Schofield]

On Sat, May 30, 2026 at 04:51:04PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Use __va(phys) instead of virt_addr + linear_offset for the kaddr
> return in __fsdev_dax_direct_access(). The previous code added a
> device-linear byte offset to virt_addr (which is __va of ranges[0]),
> but for multi-range devices with physical gaps between ranges, this
> linear arithmetic crosses the gap and produces a wrong kernel virtual
> address. Using __va(phys) where phys comes from dax_pgoff_to_phys()
> is correct for any range layout because the direct map translates
> each physical address independently.
> 
> Fixes: 759455848df0b ("dax: Save the kva from memremap")
> Signed-off-by: John Groves <john@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset

[Alison Schofield]

On Sat, May 30, 2026 at 04:51:12PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Convert the WARN_ON to a fatal error when pgmap_phys > phys. This
> condition means the remapped region starts after the device's data
> region, which is an impossible state. Previously the probe continued
> with data_offset=0, leaving virt_addr silently misaligned. Now probe
> returns -EINVAL with a diagnostic message.
> 
> Fixes: 759455848df0b ("dax: Save the kva from memremap")
> Signed-off-by: John Groves <john@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find()

[Alison Schofield]

On Sat, May 30, 2026 at 04:51:33PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> 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.
> A dax_alive() check under dax_read_lock() guards against returning a
> device that is concurrently being torn down by kill_dax().
> 
> 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@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 9/9] dax: fsdev.c minor formatting cleanup

[Alison Schofield]

On Sat, May 30, 2026 at 04:51:43PM +0000, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Address some comments from Jonathan that were missed in the merged
> series. Fix line wrapping in fsdev_dax_recovery_write() and
> fsdev_dax_zero_page_range() signatures.
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: John Groves <john@groves.net>
> ---

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[Dave Jiang]

On 5/30/26 9:50 AM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> Fix memory_failure offset calculation for multi-range devices. The old code
> subtracted ranges[0].range.start from the faulting PFN's physical address,
> which produces an incorrect (inflated) logical offset when the PFN falls in
> ranges[1] or beyond due to physical gaps between ranges. Add
> fsdev_pfn_to_offset() to walk the range list and compute the correct
> device-linear byte offset.
> 
> 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 | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 188b2526bee45..f315533b299e9 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -135,11 +135,26 @@ static void fsdev_clear_ops(void *data)
>   * The core mm code in free_zone_device_folio() handles the wake_up_var()
>   * directly for this memory type.
>   */
> +static u64 fsdev_pfn_to_offset(struct dev_dax *dev_dax, unsigned long pfn)
> +{
> +	phys_addr_t phys = PFN_PHYS(pfn);
> +	u64 offset = 0;
> +
> +	for (int i = 0; i < dev_dax->nr_range; i++) {
> +		struct range *range = &dev_dax->ranges[i].range;
> +
> +		if (phys >= range->start && phys <= range->end)
> +			return offset + (phys - range->start);
> +		offset += range_len(range);
> +	}
> +	return -1ULL;
> +}
> +
>  static int fsdev_pagemap_memory_failure(struct dev_pagemap *pgmap,
>  		unsigned long pfn, unsigned long nr_pages, int mf_flags)
>  {
>  	struct dev_dax *dev_dax = pgmap->owner;
> -	u64 offset = PFN_PHYS(pfn) - dev_dax->ranges[0].range.start;
> +	u64 offset = fsdev_pfn_to_offset(dev_dax, pfn);
>  	u64 len = nr_pages << PAGE_SHIFT;
>  
>  	return dax_holder_notify_failure(dev_dax->dax_dev, offset,