Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()

[Paul Cassella]

On Fri, 2 Jun 2023, Ira Weiny wrote:
> Paul Cassella wrote:
> > On Sat, 3 Dec 2022, Ira Weiny wrote:
> > > On Sat, Dec 03, 2022 at 09:58:58AM +0000, Yongqiang Liu wrote:

> > > > We should always call dax_region_put() whenever devm_create_dev_dax()
> > > > succeed or fail to avoid refcount leak of dax_region. Move the return
> > > > value check after dax_region_put().

> > > I think dax_region_put is called from dax_region_unregister() automatically on
> > > tear down.

> > Note the reference dax_region_unregister() will be putting is the one 
> > devm_create_dev_dax() takes by kref_get(&dax_region->kref).   I think 
> > dax_hmem_probe() needs to put its reference in the error case, as in the 
> > successful case.

> Looking at this again I'm inclined to agree that something is wrong.  But
> I'm not sure this patch fixes it. anything.

> When you say:
> 
> > ...   I think 
> > dax_hmem_probe() needs to put its reference in the error case, as in the 
> > successful case.
> 
> ... which kref_get() is dax_hmem_probe() letting go?

Isn't it letting go of the initial kref_init() reference from 
alloc_dax_region()?

Sorry, I had gone through the code a little carelessly yesterday.  Now I 
think that kref_init() reference is the one that dax_hmem_probe() is 
dropping in the success case, and which still needs to be dropped in the 
error case.

(If so, I think the alloc_dax_region() path that returns NULL on 
devm_add_action_or_reset() failure, releasing the kref_get reference, will 
leak the kref_init reference and the region.)



-- 
Paul Cassella


> Here is what I see with the current code.
> 
> dax_hmem_probe()
> 	alloc_dax_region()
> 		kref_get(&dax_region->kref)
> 		devm_add_action_or_reset(... dax_region_unregister, ...)
> 				... kref_put() later...
> 
> 	devm_create_dev_dax()
> 		...may return error...
> 		kref_get()
> 		[dev_dax_release() set to call kref_put() later]
> 		...may return error...
> 
> 	if not error
> 		dax_region_put() => kref_put()
> 
> I think this is an extra unneeded put???
> 
> Dan I see this pattern repeated in cxl and pmem.  Is the intent to remove
> the need for dax_region_unregister() to be called when the platform device
> unwinds because the platform device is not intended to own the dax_region
> after success?  If so it seems like the device managed call should be
> removed too.  Not just calling dax_region_put()?  :-/
> 
> But wouldn't that cause an issue with the sysfs entries created?
> 
> > 
> > Consider, devm_create_dev_dax() has error paths that return without 
> > involving dax_region_unregister(), prior to kref_get() and device_add().  
> > dax_hmem_probe() is clearly responsible for freeing the region in those 
> > cases.
> 
> No the devm_add_action_or_reset(... dax_region_unregister, ...) will cause
> dax_region_unregister() to release the reference when the platform device
> unwinds.
> 
> devm_create_dev_dax() configures a reference release through the
> dev_dax->type release.  So when the dev_dax device is put the dax_region
> will be released through dev_dax_release()->dax_region_put().
> 
> > 
> > 
> > dax_hmem_probe() drops its own reference in the successful case because 
> > (per the comment) "child dev_dax instances now own the lifetime of the 
> > dax_region".  That ownership is the reference that the error-case 
> > dax_region_unregister() is dropping.
> 
> No dax_region_unregister() is not just an error case flow.
> 
> >
> > dax_hmem_probe()'s initial reference 
> > also needs to be dropped in the error case, as in the successful case.
> > 
> 
> I don't follow this.  Doesn't this now result in an invalid reference
> release in dax_region_unregister() when the platform device is unwound?
> Furthermore, that reference is required I think for the sysfs entries.
> 
> > 
> > > > Fixes: c01044cc8191 ("ACPI: HMAT: refactor hmat_register_target_device to hmem_register_device")
> > > 
> > > I'm also not sure how this patch is related to this fix.
> > > 
> > > Ira
> > > 
> > > > Signed-off-by: Yongqiang Liu <liuyongqiang13@xxxxxxxxxx>
> > > > ---
> > > >  drivers/dax/hmem/hmem.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> > > > index 1bf040dbc834..09f5cd7b6c8e 100644
> > > > --- a/drivers/dax/hmem/hmem.c
> > > > +++ b/drivers/dax/hmem/hmem.c
> > > > @@ -36,12 +36,11 @@ static int dax_hmem_probe(struct platform_device *pdev)
> > > >  		.size = region_idle ? 0 : resource_size(res),
> > > >  	};
> > > >  	dev_dax = devm_create_dev_dax(&data);
> > > > -	if (IS_ERR(dev_dax))
> > > > -		return PTR_ERR(dev_dax);
> > > >  
> > > >  	/* child dev_dax instances now own the lifetime of the dax_region */
> > 
> > This comment should probably be updated now.  :)
> > 
> 
> I think removed...
> 
> Dan what do you think of this patch?  Am I seriously off the rails here?
> I worry about this code being around for so long.  But since it is in tear
> down perhaps it is just a race which has never been lost.
> 
> Ira
> 
> ---- 8< ---------------------
> 
> From f63969c761b04fb5e646e7ba7df77a48bc26ba1c Mon Sep 17 00:00:00 2001
> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> Date: Fri, 2 Jun 2023 11:17:10 -0700
> Subject: [PATCH] dax: Avoid extra kref_put() of dax_regions
> 
> In alloc_dax_region() sysfs attribute groups are created against the
> parent object the dax_region is being created under.  A reference to the
> dax_region was thus obtained for the lifetime of the parent device via
> kobj_get() and released via dax_region_unregister().
> 
> The ownership of the dax_region was intended to be transferred to the
> device dax device but this transfer is not necessary and could be
> problematic if the sysfs entries are used after the dax device is
> unwound but before the parent device is.
> 
> For the dax device the dax_region reference taken during creation in
> devm_create_dev_dax() is sufficient to ensure the dax_region lifetime
> for both the parent device and the dax device.
> 
> Remove the extraneous dax_region_put() from all call paths with this
> pattern.
> 
> Not-Yet-Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> ---
>  drivers/dax/cxl.c       | 3 ---
>  drivers/dax/hmem/hmem.c | 3 ---
>  drivers/dax/pmem.c      | 7 +------
>  3 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> index ccdf8de85bd5..d3c3654842ba 100644
> --- a/drivers/dax/cxl.c
> +++ b/drivers/dax/cxl.c
> @@ -31,9 +31,6 @@ static int cxl_dax_region_probe(struct device *dev)
>  	dev_dax = devm_create_dev_dax(&data);
>  	if (IS_ERR(dev_dax))
>  		return PTR_ERR(dev_dax);
> -
> -	/* child dev_dax instances now own the lifetime of the dax_region */
> -	dax_region_put(dax_region);
>  	return 0;
>  }
>  
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index e5fe8b39fb94..d22d56964120 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -41,9 +41,6 @@ static int dax_hmem_probe(struct platform_device *pdev)
>  	dev_dax = devm_create_dev_dax(&data);
>  	if (IS_ERR(dev_dax))
>  		return PTR_ERR(dev_dax);
> -
> -	/* child dev_dax instances now own the lifetime of the dax_region */
> -	dax_region_put(dax_region);
>  	return 0;
>  }
>  
> diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c
> index f050ea78bb83..efbdaac51e5f 100644
> --- a/drivers/dax/pmem.c
> +++ b/drivers/dax/pmem.c
> @@ -65,12 +65,7 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev)
>  		.pgmap = &pgmap,
>  		.size = range_len(&range),
>  	};
> -	dev_dax = devm_create_dev_dax(&data);
> -
> -	/* child dev_dax instances now own the lifetime of the dax_region */
> -	dax_region_put(dax_region);
> -
> -	return dev_dax;
> +	return devm_create_dev_dax(&data);
>  }
>  
>  static int dax_pmem_probe(struct device *dev)
> -- 
> 2.40.0
> 
>