Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()
Paul Cassella wrote: > 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 +, 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(_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.) Yes, *this* one looks valid. All the other patches in this thread had me asking, "um, did you try it?". Now, that said, the games that this init path is playing are hard to follow and it should not be the case that alloc_dax_region() needs to hold a reference on behalf of some future code path that might need it. Lets make this clearer by moving the reference count management closer to the object that needs it dax_region->ida. Where the extra references were being taken on behalf of "static" regions just in case they needed to be reference in dev_dax_release(). I also found a dax_mapping lifetime issue while testing this, so I appreciate the focus here.
[PATCH RFC 4/4] dax/bus: Remove unnecessary reference in alloc_dax_region()
All the callers to alloc_dax_region() maintain the device associated
with the dax_region until the dax_region is referenced by the dax_dev
they are creating.
Remove the extra kref that alloc_dax_region() takes. Add a comment to
clarify the reference counting should additional callers be grown later.
Cc: Yongqiang Liu
Cc: Dan Williams
Cc: Paul Cassella
Signed-off-by: Ira Weiny
---
drivers/dax/bus.c | 13 ++---
drivers/dax/cxl.c | 4
drivers/dax/hmem/hmem.c | 3 ---
drivers/dax/pmem.c | 8 +---
4 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 899e29d107b4..ed34d8aa6b26 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -583,7 +583,11 @@ static void dax_region_unregister(void *region)
dax_region_put(dax_region);
}
-/* The dax_region reference returned should be dropped with dax_region_put() */
+/*
+ * Caller is responsible to ensure the parent device stays live while the
+ * returned dax_region is in use. Or as is typically the case, a separate
+ * reference should be taken.
+ */
struct dax_region *alloc_dax_region(struct device *parent, int region_id,
struct range *range, int target_node, unsigned int align,
unsigned long flags)
@@ -626,13 +630,8 @@ struct dax_region *alloc_dax_region(struct device *parent,
int region_id,
return NULL;
}
- /* Hold a reference to return to the caller */
- kref_get(_region->kref);
- if (devm_add_action_or_reset(parent, dax_region_unregister,
-dax_region)) {
- kref_put(_region->kref, dax_region_free);
+ if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
return NULL;
- }
return dax_region;
}
EXPORT_SYMBOL_GPL(alloc_dax_region);
diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
index bbfe71cf4325..5ad600ee68b3 100644
--- a/drivers/dax/cxl.c
+++ b/drivers/dax/cxl.c
@@ -29,10 +29,6 @@ static int cxl_dax_region_probe(struct device *dev)
.size = range_len(_dax->hpa_range),
};
dev_dax = devm_create_dev_dax();
-
- /* child dev_dax instances now own the lifetime of the dax_region */
- dax_region_put(dax_region);
-
return IS_ERR(dev_dax) ? PTR_ERR(dev_dax) : 0;
}
diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
index b4831a3d3934..46e1b343f26e 100644
--- a/drivers/dax/hmem/hmem.c
+++ b/drivers/dax/hmem/hmem.c
@@ -39,9 +39,6 @@ static int dax_hmem_probe(struct platform_device *pdev)
.size = region_idle ? 0 : range_len(>range),
};
dev_dax = devm_create_dev_dax();
-
- /* child dev_dax instances now own the lifetime of the dax_region */
- dax_region_put(dax_region);
return IS_ERR(dev_dax) ? PTR_ERR(dev_dax) : 0;
}
diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c
index f050ea78bb83..a4f016d7f4f5 100644
--- a/drivers/dax/pmem.c
+++ b/drivers/dax/pmem.c
@@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev)
int rc, id, region_id;
resource_size_t offset;
struct nd_pfn_sb *pfn_sb;
- struct dev_dax *dev_dax;
struct dev_dax_data data;
struct nd_namespace_io *nsio;
struct dax_region *dax_region;
@@ -65,12 +64,7 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev)
.pgmap = ,
.size = range_len(),
};
- dev_dax = devm_create_dev_dax();
-
- /* 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();
}
static int dax_pmem_probe(struct device *dev)
--
2.40.0
[PATCH RFC 3/4] dax/cxl: Fix refcount leak in cxl_dax_region_probe()
alloc_dax_region() returns a reference protected dax_region. Regardless
of the success of the devm_create_dev_dax() the reference returned from
alloc_dax_region() needs to be released.
Drop the dax_region reference regardless of the success of dev_dax
creation. Clean up comments.
Fixes: 09d09e04d2fc ("cxl/dax: Create dax devices for CXL RAM regions")
Cc: Dan Williams
Cc: linux-...@vger.kernel.org
Signed-off-by: Ira Weiny
---
This work was inspired by Yongqiang Liu here:
https://lore.kernel.org/all/20221203095858.612027-1-liuyongqian...@huawei.com/
---
drivers/dax/cxl.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
index ccdf8de85bd5..bbfe71cf4325 100644
--- a/drivers/dax/cxl.c
+++ b/drivers/dax/cxl.c
@@ -29,12 +29,11 @@ static int cxl_dax_region_probe(struct device *dev)
.size = range_len(_dax->hpa_range),
};
dev_dax = devm_create_dev_dax();
- 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;
+
+ return IS_ERR(dev_dax) ? PTR_ERR(dev_dax) : 0;
}
static struct cxl_driver cxl_dax_region_driver = {
--
2.40.0
[PATCH RFC 1/4] dax/bus: Fix leaked reference in alloc_dax_region()
kref_init() initializes the ref count to 1. An extra kref is taken on
the dax_region to be used by the caller. If devm_add_action_or_reset()
fails this extra reference is leaked.
Drop the extra reference on error.
Fixes: d7fe1a67f658 ("dax: add region 'id', 'size', and 'align' attributes")
Cc: Dan Williams
Signed-off-by: Ira Weiny
---
drivers/dax/bus.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index 227800053309..899e29d107b4 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -583,6 +583,7 @@ static void dax_region_unregister(void *region)
dax_region_put(dax_region);
}
+/* The dax_region reference returned should be dropped with dax_region_put() */
struct dax_region *alloc_dax_region(struct device *parent, int region_id,
struct range *range, int target_node, unsigned int align,
unsigned long flags)
@@ -625,9 +626,13 @@ struct dax_region *alloc_dax_region(struct device *parent,
int region_id,
return NULL;
}
+ /* Hold a reference to return to the caller */
kref_get(_region->kref);
- if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
+ if (devm_add_action_or_reset(parent, dax_region_unregister,
+dax_region)) {
+ kref_put(_region->kref, dax_region_free);
return NULL;
+ }
return dax_region;
}
EXPORT_SYMBOL_GPL(alloc_dax_region);
--
2.40.0
[PATCH RFC 2/4] dax/hmem: Fix refcount leak in dax_hmem_probe()
From: Yongqiang Liu
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().
Cc: nvd...@lists.linux.dev
Fixes: c01044cc8191 ("ACPI: HMAT: refactor hmat_register_target_device to
hmem_register_device")
Reviewed-by: Ira Weiny
Signed-off-by: Yongqiang Liu
---
drivers/dax/hmem/hmem.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
index e5fe8b39fb94..b4831a3d3934 100644
--- a/drivers/dax/hmem/hmem.c
+++ b/drivers/dax/hmem/hmem.c
@@ -39,12 +39,10 @@ static int dax_hmem_probe(struct platform_device *pdev)
.size = region_idle ? 0 : range_len(>range),
};
dev_dax = devm_create_dev_dax();
- 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;
+ return IS_ERR(dev_dax) ? PTR_ERR(dev_dax) : 0;
}
static struct platform_driver dax_hmem_driver = {
--
2.40.0
[PATCH RFC 0/4] dax: Clean up dax_region references
In[*] Yongqiang Liu presented a fix to the reference counting associated with the dax_region in hmem. At the time it was thought the patch was unnecessary because dax_region_unregister() call would properly handle reference counting. Upon closer inspection Paul noted that this was not the case. In fact Yongqiang's patch was correct but there were other issues as well. This series includes Yongqiang's patch and breaks up additional fixes which can be backported if necessary followed by a final patch which simplifies the reference counting. [*] https://lore.kernel.org/all/20221203095858.612027-1-liuyongqian...@huawei.com/ Signed-off-by: Ira Weiny --- Ira Weiny (3): dax/bus: Fix leaked reference in alloc_dax_region() dax/cxl: Fix refcount leak in cxl_dax_region_probe() dax/bus: Remove unnecessary reference in alloc_dax_region() Yongqiang Liu (1): dax/hmem: Fix refcount leak in dax_hmem_probe() drivers/dax/bus.c | 6 +- drivers/dax/cxl.c | 7 +-- drivers/dax/hmem/hmem.c | 7 +-- drivers/dax/pmem.c | 8 +--- 4 files changed, 8 insertions(+), 20 deletions(-) --- base-commit: 921bdc72a0d68977092d6a64855a1b8967acc1d9 change-id: 20230602-dax-region-put-72c289137cb1 Best regards, -- Ira Weiny
Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()
Paul Cassella wrote: > 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 +, 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(_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()? Oh wow! I did not realize that about kref_init()... :-( > > 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. yea ok... > > (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.) I see now. The ref is taken prior to the add action or reset to ensure the dax_region does not go away should the platform device go away... However, in all the call paths currently the device passed to alloc_dax_region() can't go away prior to the dev_dax taking a reference. So I don't think this extra reference is required. :-/ As you say the ref counting right now is not correct on error flows. But I'm torn on the correct solution. I think a small series broken out so it can be backported if needed would be best. Ira
Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()
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 +, 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(_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(_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
> > > > ---
> > > > 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();
> > > > - 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
Re: [PATCH] dax/hmem: Fix refcount leak in dax_hmem_probe()
Paul Cassella wrote:
> On Sat, 3 Dec 2022, Ira Weiny wrote:
> > On Sat, Dec 03, 2022 at 09:58:58AM +, 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.
>
> Hi Ira,
>
> Note the reference dax_region_unregister() will be putting is the one
> devm_create_dev_dax() takes by kref_get(_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?
Here is what I see with the current code.
dax_hmem_probe()
alloc_dax_region()
kref_get(_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
> > > ---
> > > 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();
> > > - 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
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
