* Re: [PATCH] nvdimm: pmem: fix gendisk leak when badblocks init fails
2026-08-24 6:01 [PATCH] nvdimm: pmem: fix gendisk leak when badblocks init fails Hemanth Selam
@ 2026-08-27 22:51 ` Alison Schofield
0 siblings, 0 replies; 2+ messages in thread
From: Alison Schofield @ 2026-08-27 22:51 UTC (permalink / raw)
To: Hemanth Selam
Cc: djbw, vishal.l.verma, dave.jiang, iweiny, nvdimm, linux-kernel
On Mon, Aug 24, 2026 at 11:31:13AM +0530, Hemanth Selam wrote:
Hi Hemanth,
Thanks for the patch. The fix itself looks right but the changelog made
this harder to review than it needed to be for a simple fixup.
I am fine with an AI tool finding a correctness issue, and I like that
you found a way to exercise and test the change, yet neither needs to
be dressed up as something else.
Suggestions for a v2:
nvdimm/pmem: Release gendisk on probe failure
> pmem_attach_disk() allocates the gendisk with blk_alloc_disk() and only
> hands it over to devres at the very end, after device_add_disk() has
> succeeded:
>
> if (devm_add_action_or_reset(dev, pmem_release_disk, pmem))
> return -ENOMEM;
>
> Every error path in between either has nothing to release yet or jumps
> to the out: label, which drops the last reference with put_disk(). The
> devm_init_badblocks() failure returns directly instead, so on that path
> the gendisk, its queue and its bdev inode are never freed.
This mostly walks through the code. Please structure the changelog as
background, problem, impact, and resolution, without narrating the call
sequence or error labels.
For example:
pmem namespace probe allocates a gendisk before initializing its
badblocks state. If that initialization fails, probe returns without
releasing the gendisk.
<Describe when this failure can realistically occur and what happens
to the leaked resources. Is the gendisk permanently leaked, or is it
eventually released when the namespace is unbound or destroyed? Is
anything user-visible left behind?>
Release the gendisk through the existing cleanup path on this failure.
> The disk has
> been allocated before this check since the check was introduced, so the
> leak is as old as the check itself.
I don't think that establishes the origin of the bug. Please look at the
code immediately before b95f5f4391fa and re-derive the Fixes: tag.
>
> Forcing the branch and rebinding a namespace 64 times shows one gendisk
> leaked per failed probe: disk_release() is never reached and bdev_cache
> grows by 60 objects. With the goto, disk_release() runs on every
> attempt and bdev_cache returns to its original size.
I like that you found a way to test the error path. Please state how
you did that, what tool? Make it clear that this is a unit test of a
difficult-to-reach error path, not a user scenario.
Also, 64 failed probes resulting in 60 additional objects caught my eye.
If that difference is meaningful, explain it. Otherwise, I would leave
that level of detail out.
Likewise, please say how the bug was found.
-- Alison
>
> Fixes: b95f5f4391fa ("libnvdimm: convert to statically allocated badblocks")
> Signed-off-by: Hemanth Selam <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.43.7
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread