* [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series [not found] <20260530164953.6578-1-john@jagalactic.com> @ 2026-05-30 16:50 ` John Groves [not found] ` <20260530165029.6601-1-john@jagalactic.com> ` (9 more replies) 0 siblings, 10 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:50 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165029.6601-1-john@jagalactic.com>]
* [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() [not found] ` <20260530165029.6601-1-john@jagalactic.com> @ 2026-05-30 16:50 ` John Groves 2026-06-01 21:51 ` Dave Jiang 2026-06-03 0:12 ` Alison Schofield 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:50 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() 2026-05-30 16:50 ` [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() John Groves @ 2026-06-01 21:51 ` Dave Jiang 2026-06-03 0:12 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-01 21:51 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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; ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() 2026-05-30 16:50 ` [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() John Groves 2026-06-01 21:51 ` Dave Jiang @ 2026-06-03 0:12 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:12 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165037.6619-1-john@jagalactic.com>]
* [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler [not found] ` <20260530165037.6619-1-john@jagalactic.com> @ 2026-05-30 16:50 ` John Groves 2026-06-03 0:12 ` Alison Schofield 2026-06-03 0:17 ` Dave Jiang 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:50 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler 2026-05-30 16:50 ` [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler John Groves @ 2026-06-03 0:12 ` Alison Schofield 2026-06-03 0:17 ` Dave Jiang 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:12 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler 2026-05-30 16:50 ` [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler John Groves 2026-06-03 0:12 ` Alison Schofield @ 2026-06-03 0:17 ` Dave Jiang 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-03 0:17 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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, ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165045.6636-1-john@jagalactic.com>]
* [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap [not found] ` <20260530165045.6636-1-john@jagalactic.com> @ 2026-05-30 16:50 ` John Groves 2026-06-01 22:06 ` Dave Jiang 2026-06-03 0:13 ` Alison Schofield 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:50 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap 2026-05-30 16:50 ` [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap John Groves @ 2026-06-01 22:06 ` Dave Jiang 2026-06-03 0:13 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-01 22:06 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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; > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap 2026-05-30 16:50 ` [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap John Groves 2026-06-01 22:06 ` Dave Jiang @ 2026-06-03 0:13 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:13 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165053.6653-1-john@jagalactic.com>]
* [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure [not found] ` <20260530165053.6653-1-john@jagalactic.com> @ 2026-05-30 16:50 ` John Groves 2026-06-01 23:10 ` Dave Jiang 2026-06-03 0:14 ` Alison Schofield 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:50 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure 2026-05-30 16:50 ` [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure John Groves @ 2026-06-01 23:10 ` Dave Jiang 2026-06-03 0:14 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-01 23:10 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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 = { ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure 2026-05-30 16:50 ` [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure John Groves 2026-06-01 23:10 ` Dave Jiang @ 2026-06-03 0:14 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:14 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165100.6670-1-john@jagalactic.com>]
* [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access [not found] ` <20260530165100.6670-1-john@jagalactic.com> @ 2026-05-30 16:51 ` John Groves 2026-06-01 23:24 ` Dave Jiang 2026-06-03 0:14 ` Alison Schofield 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:51 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access 2026-05-30 16:51 ` [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access John Groves @ 2026-06-01 23:24 ` Dave Jiang 2026-06-03 0:14 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-01 23:24 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access 2026-05-30 16:51 ` [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access John Groves 2026-06-01 23:24 ` Dave Jiang @ 2026-06-03 0:14 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:14 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165107.6687-1-john@jagalactic.com>]
* [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset [not found] ` <20260530165107.6687-1-john@jagalactic.com> @ 2026-05-30 16:51 ` John Groves 2026-06-03 0:15 ` Alison Schofield 0 siblings, 1 reply; 27+ messages in thread From: John Groves @ 2026-05-30 16:51 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset 2026-05-30 16:51 ` [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset John Groves @ 2026-06-03 0:15 ` Alison Schofield 0 siblings, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:15 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165115.6704-1-john@jagalactic.com>]
* [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax() [not found] ` <20260530165115.6704-1-john@jagalactic.com> @ 2026-05-30 16:51 ` John Groves 2026-06-02 0:03 ` Dave Jiang 0 siblings, 1 reply; 27+ messages in thread From: John Groves @ 2026-05-30 16:51 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax() 2026-05-30 16:51 ` [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax() John Groves @ 2026-06-02 0:03 ` Dave Jiang 2026-06-07 15:37 ` John Groves 0 siblings, 1 reply; 27+ messages in thread From: Dave Jiang @ 2026-06-02 0:03 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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); } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax() 2026-06-02 0:03 ` Dave Jiang @ 2026-06-07 15:37 ` John Groves 0 siblings, 0 replies; 27+ messages in thread From: John Groves @ 2026-06-07 15:37 UTC (permalink / raw) To: Dave Jiang Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel On 26/06/01 05:03PM, Dave Jiang wrote: > > > 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); > } > > Looks good - going with this approach. Thanks! John ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165126.6721-1-john@jagalactic.com>]
* [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() [not found] ` <20260530165126.6721-1-john@jagalactic.com> @ 2026-05-30 16:51 ` John Groves 2026-06-02 0:13 ` Dave Jiang 2026-06-03 0:15 ` Alison Schofield 0 siblings, 2 replies; 27+ messages in thread From: John Groves @ 2026-05-30 16:51 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() 2026-05-30 16:51 ` [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() John Groves @ 2026-06-02 0:13 ` Dave Jiang 2026-06-03 0:15 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Dave Jiang @ 2026-06-02 0:13 UTC (permalink / raw) To: John Groves, John Groves, Dan Williams Cc: John Groves, Vishal Verma, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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) > { > } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() 2026-05-30 16:51 ` [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() John Groves 2026-06-02 0:13 ` Dave Jiang @ 2026-06-03 0:15 ` Alison Schofield 1 sibling, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:15 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <20260530165135.6738-1-john@jagalactic.com>]
* [PATCH V3 9/9] dax: fsdev.c minor formatting cleanup [not found] ` <20260530165135.6738-1-john@jagalactic.com> @ 2026-05-30 16:51 ` John Groves 2026-06-03 0:16 ` Alison Schofield 0 siblings, 1 reply; 27+ messages in thread From: John Groves @ 2026-05-30 16:51 UTC (permalink / raw) To: John Groves, Dan Williams Cc: John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Alison Schofield, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel, 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 9/9] dax: fsdev.c minor formatting cleanup 2026-05-30 16:51 ` [PATCH V3 9/9] dax: fsdev.c minor formatting cleanup John Groves @ 2026-06-03 0:16 ` Alison Schofield 0 siblings, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:16 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series 2026-05-30 16:50 ` [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series John Groves ` (8 preceding siblings ...) [not found] ` <20260530165135.6738-1-john@jagalactic.com> @ 2026-06-03 0:11 ` Alison Schofield 9 siblings, 0 replies; 27+ messages in thread From: Alison Schofield @ 2026-06-03 0:11 UTC (permalink / raw) To: John Groves Cc: John Groves, Dan Williams, John Groves, Vishal Verma, Dave Jiang, Matthew Wilcox, Jan Kara, Alexander Viro, Christian Brauner, Miklos Szeredi, Ira Weiny, Jonathan Cameron, nvdimm, linux-cxl, linux-kernel, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2026-06-07 15:37 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20260530164953.6578-1-john@jagalactic.com>
2026-05-30 16:50 ` [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series John Groves
[not found] ` <20260530165029.6601-1-john@jagalactic.com>
2026-05-30 16:50 ` [PATCH V3 1/9] dax: fix misleading comment about share/index union in dax_folio_reset_order() John Groves
2026-06-01 21:51 ` Dave Jiang
2026-06-03 0:12 ` Alison Schofield
[not found] ` <20260530165037.6619-1-john@jagalactic.com>
2026-05-30 16:50 ` [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler John Groves
2026-06-03 0:12 ` Alison Schofield
2026-06-03 0:17 ` Dave Jiang
[not found] ` <20260530165045.6636-1-john@jagalactic.com>
2026-05-30 16:50 ` [PATCH V3 3/9] dax/fsdev: clear vmemmap_shift when binding static pgmap John Groves
2026-06-01 22:06 ` Dave Jiang
2026-06-03 0:13 ` Alison Schofield
[not found] ` <20260530165053.6653-1-john@jagalactic.com>
2026-05-30 16:50 ` [PATCH V3 4/9] dax/fsdev: clear dev_dax->pgmap on probe failure John Groves
2026-06-01 23:10 ` Dave Jiang
2026-06-03 0:14 ` Alison Schofield
[not found] ` <20260530165100.6670-1-john@jagalactic.com>
2026-05-30 16:51 ` [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access John Groves
2026-06-01 23:24 ` Dave Jiang
2026-06-03 0:14 ` Alison Schofield
[not found] ` <20260530165107.6687-1-john@jagalactic.com>
2026-05-30 16:51 ` [PATCH V3 6/9] dax/fsdev: fail probe on invalid pgmap offset John Groves
2026-06-03 0:15 ` Alison Schofield
[not found] ` <20260530165115.6704-1-john@jagalactic.com>
2026-05-30 16:51 ` [PATCH V3 7/9] dax: fix holder_ops race in fs_put_dax() John Groves
2026-06-02 0:03 ` Dave Jiang
2026-06-07 15:37 ` John Groves
[not found] ` <20260530165126.6721-1-john@jagalactic.com>
2026-05-30 16:51 ` [PATCH V3 8/9] dax: replace exported dax_dev_get() with non-allocating dax_dev_find() John Groves
2026-06-02 0:13 ` Dave Jiang
2026-06-03 0:15 ` Alison Schofield
[not found] ` <20260530165135.6738-1-john@jagalactic.com>
2026-05-30 16:51 ` [PATCH V3 9/9] dax: fsdev.c minor formatting cleanup John Groves
2026-06-03 0:16 ` Alison Schofield
2026-06-03 0:11 ` [PATCH V3 0/9] Fixes to the previously-merged drivers/dax/fsdev series Alison Schofield
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome