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@xxxxxxxxx>

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

> ---
> 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@xxxxxxxxx/
> 
>  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;
>