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 <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> 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/[email protected]/
>
> 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;
>
