[PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[Hemanth Selam]

pmem_attach_disk() allocates the gendisk with blk_alloc_disk() and hands
it to devres only once device_add_disk() has succeeded.  Until that
point the probe path owns the disk itself, which is why every failure
after the allocation jumps to the out: label and puts it there.

The devm_init_badblocks() failure returns directly instead, so the disk
allocated a few lines earlier is never released.  Nothing releases it
afterwards either: the devres action that would have done so has not
been registered yet, so unbinding the namespace or destroying it does
not reach the disk, and it stays allocated along with its queue and its
bdev inode until the machine is rebooted.

devm_init_badblocks() only fails when a single page allocation fails, so
reaching this at all needs memory exhaustion during namespace probe, and
because device_add_disk() has not run there is nothing user visible left
behind: no device node, no sysfs entry, only the leaked memory.

Release the gendisk through the existing cleanup path on this failure.

Fixes: 3dd60fb9d95d ("nvdimm/pmem: stop using q_usage_count as external pgmap refcount")
Signed-off-by: Hemanth Selam <hemanth.selam@gmail.com>
---
v2, all of it from Alison's review of v1:
 - retitled, and the changelog rewritten as background, problem, impact
   and resolution rather than a walk through the call sequence
 - says whether the disk is permanently leaked: it is, because the
   devres action has not been registered at that point, so no later
   unbind or destroy reaches it
 - says when the failure can be reached at all, and that nothing user
   visible is left behind
 - the Fixes: tag re-derived.  v1 blamed b95f5f4391fa, but the early
   return after the disk was allocated already existed before it; that
   commit only changed which call failed.  The leak starts at
   3dd60fb9d95d, which removed the pmem_release_queue devres action and
   the fsdax_pagemap_ops .cleanup that had been freeing the disk on
   these paths.  accf58afb689 then converted the addr and dax_dev
   returns to goto out, and this one was missed.
 - the object counts kept, but measured across four batch sizes so that
   the scaling is visible, and the shortfall you noticed explained

Found by an AI-assisted review of the error paths in pmem_attach_disk().

Tested on 7.3.0-rc2 in QEMU, with a legacy pmem region (memmap=1G!2G) and
a local debug patch forcing the devm_init_badblocks() branch, as it is
otherwise only reachable under memory exhaustion.  namespace0.0 was bound
and unbound repeatedly with the branch forced, counting bdev_cache in
/proc/slabinfo after a drop_caches and a settle:

    failed probes      32     64    128    256
    growth, unfixed   +24    +60   +120   +252
    growth, fixed     +12    +12    +12    +12

Without the patch the count tracks the number of failed probes, and the
unbind between attempts does not bring it back down, which is what makes
the leak permanent.  With the patch it is flat.  Both rows sit a little
under the probe count because SLUB's active_objs is an estimate, which is
the discrepancy you asked about in v1.  Clearing the debug flag and
binding again still gives a working /dev/pmem0.

v1: https://lore.kernel.org/all/20260824060113.2330468-1-hemanth.selam@gmail.com/

 drivers/nvdimm/pmem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 30a51c365ce8..648fc7d66063 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -563,8 +563,10 @@ static int pmem_attach_disk(struct device *dev,
 	nvdimm_namespace_disk_name(ndns, disk->disk_name);
 	set_capacity(disk, (pmem->size - pmem->pfn_pad - pmem->data_offset)
 			/ 512);
-	if (devm_init_badblocks(dev, &pmem->bb))
-		return -ENOMEM;
+	if (devm_init_badblocks(dev, &pmem->bb)) {
+		rc = -ENOMEM;
+		goto out;
+	}
 	nvdimm_badblocks_populate(nd_region, &pmem->bb, &bb_range);
 	disk->bb = &pmem->bb;
 
-- 
2.48.1

Re: [PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[Dave Jiang]

On 9/11/26 1:25 AM, Hemanth Selam wrote:
> pmem_attach_disk() allocates the gendisk with blk_alloc_disk() and hands
> it to devres only once device_add_disk() has succeeded.  Until that
> point the probe path owns the disk itself, which is why every failure
> after the allocation jumps to the out: label and puts it there.
> 
> The devm_init_badblocks() failure returns directly instead, so the disk
> allocated a few lines earlier is never released.  Nothing releases it
> afterwards either: the devres action that would have done so has not
> been registered yet, so unbinding the namespace or destroying it does
> not reach the disk, and it stays allocated along with its queue and its
> bdev inode until the machine is rebooted.
> 
> devm_init_badblocks() only fails when a single page allocation fails, so
> reaching this at all needs memory exhaustion during namespace probe, and
> because device_add_disk() has not run there is nothing user visible left
> behind: no device node, no sysfs entry, only the leaked memory.
> 
> Release the gendisk through the existing cleanup path on this failure.
> 
> Fixes: 3dd60fb9d95d ("nvdimm/pmem: stop using q_usage_count as external pgmap refcount")
> Signed-off-by: Hemanth Selam <hemanth.selam@gmail.com>

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

> ---
> v2, all of it from Alison's review of v1:
>  - retitled, and the changelog rewritten as background, problem, impact
>    and resolution rather than a walk through the call sequence
>  - says whether the disk is permanently leaked: it is, because the
>    devres action has not been registered at that point, so no later
>    unbind or destroy reaches it
>  - says when the failure can be reached at all, and that nothing user
>    visible is left behind
>  - the Fixes: tag re-derived.  v1 blamed b95f5f4391fa, but the early
>    return after the disk was allocated already existed before it; that
>    commit only changed which call failed.  The leak starts at
>    3dd60fb9d95d, which removed the pmem_release_queue devres action and
>    the fsdax_pagemap_ops .cleanup that had been freeing the disk on
>    these paths.  accf58afb689 then converted the addr and dax_dev
>    returns to goto out, and this one was missed.
>  - the object counts kept, but measured across four batch sizes so that
>    the scaling is visible, and the shortfall you noticed explained
> 
> Found by an AI-assisted review of the error paths in pmem_attach_disk().
> 
> Tested on 7.3.0-rc2 in QEMU, with a legacy pmem region (memmap=1G!2G) and
> a local debug patch forcing the devm_init_badblocks() branch, as it is
> otherwise only reachable under memory exhaustion.  namespace0.0 was bound
> and unbound repeatedly with the branch forced, counting bdev_cache in
> /proc/slabinfo after a drop_caches and a settle:
> 
>     failed probes      32     64    128    256
>     growth, unfixed   +24    +60   +120   +252
>     growth, fixed     +12    +12    +12    +12
> 
> Without the patch the count tracks the number of failed probes, and the
> unbind between attempts does not bring it back down, which is what makes
> the leak permanent.  With the patch it is flat.  Both rows sit a little
> under the probe count because SLUB's active_objs is an estimate, which is
> the discrepancy you asked about in v1.  Clearing the debug flag and
> binding again still gives a working /dev/pmem0.
> 
> v1: https://lore.kernel.org/all/20260824060113.2330468-1-hemanth.selam@gmail.com/
> 
>  drivers/nvdimm/pmem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 30a51c365ce8..648fc7d66063 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -563,8 +563,10 @@ static int pmem_attach_disk(struct device *dev,
>  	nvdimm_namespace_disk_name(ndns, disk->disk_name);
>  	set_capacity(disk, (pmem->size - pmem->pfn_pad - pmem->data_offset)
>  			/ 512);
> -	if (devm_init_badblocks(dev, &pmem->bb))
> -		return -ENOMEM;
> +	if (devm_init_badblocks(dev, &pmem->bb)) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  	nvdimm_badblocks_populate(nd_region, &pmem->bb, &bb_range);
>  	disk->bb = &pmem->bb;
>  

[PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[Hemanth Selam]

pmem namespace probe allocates a gendisk early, fills it in, and only hands
it to devres once device_add_disk() has succeeded.  Until that handover the
probe itself owns the disk, and the error paths in between release it
through a common label.

Initializing the namespace's badblocks state sits between those two points
but returns directly on failure, so the gendisk, its queue and its bdev
inode are left allocated with nothing to free them.  The devres release
action has not been registered at that point, so unbinding or destroying
the namespace does not reclaim them either, and they stay allocated for the
rest of the boot.  Nothing is user visible, as the disk was never added, so
the only effect is the memory.

devm_init_badblocks() fails only if a one page GFP_KERNEL allocation fails,
which needs memory exhaustion, so the path is hard to reach in practice.

Release the gendisk through the existing cleanup path on this failure.

Fixes: 0caeef63e6d2 ("libnvdimm: Add a poison list and export badblocks")
Assisted-by: Cursor:claude-opus-5
Signed-off-by: Hemanth Selam <hemanth.selam@gmail.com>
---
Changes in v2, all from Alison's review of v1:
 - Retitled, and the changelog rewritten as background, problem, impact and
   resolution rather than a walk through the error labels.
 - Fixes: re-derived.  b95f5f4391fa only replaced one failing call with
   another; the early return that leaks the disk was added a week earlier by
   0caeef63e6d2, when the disk was already allocated by alloc_disk_node()
   and the only put_disk() was in the detach path.
 - Says what the leak's fate is: the devres release action is registered
   only after device_add_disk(), so nothing reclaims the disk on unbind.
 - The object counts are gone, see below for why.

How this was found: reviewing pmem's probe error paths with an AI assistant,
hence the Assisted-by tag.  The finding was then confirmed against the code
and measured as follows.

How it was tested: this is a unit test of an error path that needs memory
exhaustion to reach, not a user visible scenario.  The badblocks init in
pmem_attach_disk() was forced to fail with a throwaway kernel change, a
namespace was bound 32 times in a guest with memmap=256M!1G, and
disk_release() calls were counted with ftrace's function profiler:

                          without the patch   with the patch
  disk_release() calls          0 of 32          32 of 32
  bdev_cache active         48 -> 72 (+24)     48 -> 48 (+0)

v1 quoted the bdev_cache delta, which is what drew the question about 64
attempts and 60 objects: slab accounting does not track this one for one,
as the +24 above shows for 32 probes.  disk_release() does, so that is the
number reported here and the slab figure is only context.

 drivers/nvdimm/pmem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 30a51c365ce8..648fc7d66063 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -563,8 +563,10 @@ static int pmem_attach_disk(struct device *dev,
 	nvdimm_namespace_disk_name(ndns, disk->disk_name);
 	set_capacity(disk, (pmem->size - pmem->pfn_pad - pmem->data_offset)
 			/ 512);
-	if (devm_init_badblocks(dev, &pmem->bb))
-		return -ENOMEM;
+	if (devm_init_badblocks(dev, &pmem->bb)) {
+		rc = -ENOMEM;
+		goto out;
+	}
 	nvdimm_badblocks_populate(nd_region, &pmem->bb, &bb_range);
 	disk->bb = &pmem->bb;
 
-- 
2.43.7

Re: [PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[Hemanth Selam]

On Mon, Aug 31, 2026 at 04:45:08AM +0000, sashiko-bot@kernel.org wrote:
> This isn't a bug introduced by this patch, but I wanted to ask about the error
> handling for alloc_dax() just below this code.
>
> In pmem_attach_disk(), if alloc_dax() fails with -EOPNOTSUPP, the code
> gracefully falls back and leaves pmem->dax_dev as NULL:
[...]
> However, the write_cache sysfs attribute remains unconditionally visible as long
> as CONFIG_ARCH_HAS_PMEM_API is enabled.

I do not think the two can meet.  alloc_dax() only returns -EOPNOTSUPP for

	if (ops && cpu_dcache_is_aliasing())
		return ERR_PTR(-EOPNOTSUPP);

so pmem->dax_dev is left NULL only on architectures whose data caches are
virtually aliased: csky, m68k, parisc and sh unconditionally, arm and mips
depending on the CPU.

write_cache is visible only when CONFIG_ARCH_HAS_PMEM_API is selected, and
that is arm64, powerpc, riscv and x86_64 (X86_64 only), none of which have
aliasing data caches.  So on every architecture where alloc_dax() can hand
back -EOPNOTSUPP, dax_visible() returns 0 for write_cache and there is
nothing to read.

Happy to be shown otherwise if there is a configuration I am missing, in
which case the fix belongs in dax_visible() rather than in this patch.

Hemanth