* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-05 2:22 [PATCH] resource: Fix resource leak in get_free_mem_region() Dan Williams
@ 2025-03-05 2:54 ` Zhijian Li (Fujitsu)
2025-03-05 3:06 ` Dan Williams
2025-03-05 10:22 ` Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Zhijian Li (Fujitsu) @ 2025-03-05 2:54 UTC (permalink / raw)
To: Dan Williams, akpm@linux-foundation.org
Cc: linux-cxl@vger.kernel.org, Jonathan.Cameron@huawei.com,
andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, ilpo.jarvinen@linux.intel.com,
bhelgaas@google.com, huang.ying.caritas@gmail.com
On 05/03/2025 10:22, Dan Williams wrote:
> Li reports a kmemleak detection in get_free_mem_region() error unwind
> path:
>
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> It turns out it not only leaks memory, also fails to unwind changes to
> the resource tree (@base, usually iomem_resource).
Make sense!
>
> Fix this by consolidating the devres and devm paths into just devres,
> and move those details to a wrapper function. So now
> __get_free_mem_region() only needs to worry about alloc_resource()
> unwinding, and the devres failure path is resolved before touching the
> resource tree.
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Thanks for your quickly fix!
Tested-by: Li Zhijian <lizhijian@fujitsu.com>
Comment inline below:
> ---
>
> Andrew, here is a replacement patch for
> resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It
> addresses missing calls to remove_resource() in addition to
> free_resource() in the error path.
>
> kernel/resource.c | 105 ++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 60 insertions(+), 45 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 12004452d999..80d10714cb38 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -172,6 +172,8 @@ static void free_resource(struct resource *res)
> kfree(res);
> }
>
> +DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T))
> +
> static struct resource *alloc_resource(gfp_t flags)
> {
> return kzalloc(sizeof(struct resource), flags);
> @@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new)
> }
> EXPORT_SYMBOL(devm_release_resource);
>
> +/*
> + * The GFR_REQUEST_REGION case performs a request_region() to be paired
> + * with release_region(). The alloc_free_mem_region() path performs
> + * insert_resource() to be paired with {remove,free}_resource(). The
> + * @res member differentiates the 2 cases.
> + */
> struct region_devres {
> struct resource *parent;
> resource_size_t start;
> resource_size_t n;
> + struct resource *res;
> };
>
> static void devm_region_release(struct device *dev, void *res)
> {
> struct region_devres *this = res;
>
> - __release_region(this->parent, this->start, this->n);
> + if (!this->res) {
> + __release_region(this->parent, this->start, this->n);
IIUC 'this->parent' now points to base instead of the previous '&iomem_resource',
which might change earlier logical assumptions elsewhere if other code tries to
reference it
Is it interned?
Other looks good to me.
Thanks
Zhijian
> + } else {
> + remove_resource(this->res);
> + free_resource(this->res);
> + }
> }
>
> static int devm_region_match(struct device *dev, void *res, void *match_data)
> @@ -1908,43 +1922,19 @@ static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
> return addr + size;
> }
>
> -static void remove_free_mem_region(void *_res)
> -{
> - struct resource *res = _res;
> -
> - if (res->parent)
> - remove_resource(res);
> - free_resource(res);
> -}
> -
> static struct resource *
> -get_free_mem_region(struct device *dev, struct resource *base,
> - resource_size_t size, const unsigned long align,
> - const char *name, const unsigned long desc,
> - const unsigned long flags)
> +__get_free_mem_region(struct resource *base, resource_size_t size,
> + const unsigned long align, const char *name,
> + const unsigned long desc, const unsigned long flags)
> {
> resource_size_t addr;
> - struct resource *res;
> - struct region_devres *dr = NULL;
>
> size = ALIGN(size, align);
>
> - res = alloc_resource(GFP_KERNEL);
> + struct resource *res __free(free_resource) = alloc_resource(GFP_KERNEL);
> if (!res)
> return ERR_PTR(-ENOMEM);
>
> - if (dev && (flags & GFR_REQUEST_REGION)) {
> - dr = devres_alloc(devm_region_release,
> - sizeof(struct region_devres), GFP_KERNEL);
> - if (!dr) {
> - free_resource(res);
> - return ERR_PTR(-ENOMEM);
> - }
> - } else if (dev) {
> - if (devm_add_action_or_reset(dev, remove_free_mem_region, res))
> - return ERR_PTR(-ENOMEM);
> - }
> -
> write_lock(&resource_lock);
> for (addr = gfr_start(base, size, align, flags);
> gfr_continue(base, addr, align, flags);
> @@ -1958,17 +1948,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
> size, name, 0))
> break;
>
> - if (dev) {
> - dr->parent = &iomem_resource;
> - dr->start = addr;
> - dr->n = size;
> - devres_add(dev, dr);
> - }
> -
> res->desc = desc;
> write_unlock(&resource_lock);
>
> -
> /*
> * A driver is claiming this region so revoke any
> * mappings.
> @@ -1985,25 +1967,58 @@ get_free_mem_region(struct device *dev, struct resource *base,
> * Only succeed if the resource hosts an exclusive
> * range after the insert
> */
> - if (__insert_resource(base, res) || res->child)
> + if (__insert_resource(base, res))
> + break;
> + if (res->child) {
> + remove_resource(res);
> break;
> + }
>
> write_unlock(&resource_lock);
> }
>
> - return res;
> + return no_free_ptr(res);
> }
> write_unlock(&resource_lock);
>
> - if (flags & GFR_REQUEST_REGION) {
> - free_resource(res);
> - devres_free(dr);
> - } else if (dev)
> - devm_release_action(dev, remove_free_mem_region, res);
> -
> return ERR_PTR(-ERANGE);
> }
>
> +static struct resource *
> +get_free_mem_region(struct device *dev, struct resource *base,
> + resource_size_t size, const unsigned long align,
> + const char *name, const unsigned long desc,
> + const unsigned long flags)
> +{
> +
> + struct region_devres *dr __free(kfree) = NULL;
> + struct resource *res;
> +
> + if (dev) {
> + dr = devres_alloc(devm_region_release,
> + sizeof(struct region_devres), GFP_KERNEL);
> + if (!dr)
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + res = __get_free_mem_region(base, size, align, name, desc, flags);
> +
> + if (IS_ERR(res) || !dr)
> + return res;
> +
> + dr->parent = base;
> + dr->start = res->start;
> + dr->n = resource_size(res);
> +
> + /* See 'struct region_devres' definition for details */
> + if ((flags & GFR_REQUEST_REGION) == 0)
> + dr->res = res;
> +
> + devres_add(dev, no_free_ptr(dr));
> +
> + return res;
> +}
> +
> /**
> * devm_request_free_mem_region - find free region for device private memory
> *
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-05 2:54 ` Zhijian Li (Fujitsu)
@ 2025-03-05 3:06 ` Dan Williams
0 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2025-03-05 3:06 UTC (permalink / raw)
To: Zhijian Li (Fujitsu), Dan Williams, akpm@linux-foundation.org
Cc: linux-cxl@vger.kernel.org, Jonathan.Cameron@huawei.com,
andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, ilpo.jarvinen@linux.intel.com,
bhelgaas@google.com, huang.ying.caritas@gmail.com
Zhijian Li (Fujitsu) wrote:
>
>
> On 05/03/2025 10:22, Dan Williams wrote:
> > Li reports a kmemleak detection in get_free_mem_region() error unwind
> > path:
> >
> > cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> >
> > __kmalloc_cache_noprof+0x28c/0x350
> > get_free_mem_region+0x45/0x380
> > alloc_free_mem_region+0x1d/0x30
> > size_store+0x180/0x290 [cxl_core]
> > kernfs_fop_write_iter+0x13f/0x1e0
> > vfs_write+0x37c/0x540
> > ksys_write+0x68/0xe0
> > do_syscall_64+0x6e/0x190
> > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >
> > It turns out it not only leaks memory, also fails to unwind changes to
> > the resource tree (@base, usually iomem_resource).
>
> Make sense!
>
>
> >
> > Fix this by consolidating the devres and devm paths into just devres,
> > and move those details to a wrapper function. So now
> > __get_free_mem_region() only needs to worry about alloc_resource()
> > unwinding, and the devres failure path is resolved before touching the
> > resource tree.
> >
> > Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> > Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> > Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
>
> Thanks for your quickly fix!
>
> Tested-by: Li Zhijian <lizhijian@fujitsu.com>
>
> Comment inline below:
>
>
> > ---
> >
> > Andrew, here is a replacement patch for
> > resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It
> > addresses missing calls to remove_resource() in addition to
> > free_resource() in the error path.
> >
> > kernel/resource.c | 105 ++++++++++++++++++++++++++++++-----------------------
> > 1 file changed, 60 insertions(+), 45 deletions(-)
> >
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index 12004452d999..80d10714cb38 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -172,6 +172,8 @@ static void free_resource(struct resource *res)
> > kfree(res);
> > }
> >
> > +DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T))
> > +
> > static struct resource *alloc_resource(gfp_t flags)
> > {
> > return kzalloc(sizeof(struct resource), flags);
> > @@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new)
> > }
> > EXPORT_SYMBOL(devm_release_resource);
> >
> > +/*
> > + * The GFR_REQUEST_REGION case performs a request_region() to be paired
> > + * with release_region(). The alloc_free_mem_region() path performs
> > + * insert_resource() to be paired with {remove,free}_resource(). The
> > + * @res member differentiates the 2 cases.
> > + */
> > struct region_devres {
> > struct resource *parent;
> > resource_size_t start;
> > resource_size_t n;
> > + struct resource *res;
> > };
> >
> > static void devm_region_release(struct device *dev, void *res)
> > {
> > struct region_devres *this = res;
> >
> > - __release_region(this->parent, this->start, this->n);
> > + if (!this->res) {
> > + __release_region(this->parent, this->start, this->n);
>
>
> IIUC 'this->parent' now points to base instead of the previous '&iomem_resource',
> which might change earlier logical assumptions elsewhere if other code tries to
> reference it
>
> Is it interned?
Good question, it is intentional. I had the same concern that someone
might have been dependent on it pointing to &iomem_resource even though
@base is some other resource.
However, if you take a look, all of the request_free_mem_region()
callers set @base to &iomem_resource.
Now, alloc_free_mem_region() *does* arrange for @base to be "cxlrd->res"
(not &iomem_resource), but prior to this change no 'struct
region_devres' was allocated for the alloc_free_mem_region() path. So
there is no risk that something could have depended on "dr->parent ==
&iomem_resource" when @base is "cxlrd->res".
> Other looks good to me.
Thanks for the review, and the report!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-05 2:22 [PATCH] resource: Fix resource leak in get_free_mem_region() Dan Williams
2025-03-05 2:54 ` Zhijian Li (Fujitsu)
@ 2025-03-05 10:22 ` Andy Shevchenko
2025-03-14 11:49 ` Jonathan Cameron
2025-06-04 6:06 ` Zhijian Li (Fujitsu)
3 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2025-03-05 10:22 UTC (permalink / raw)
To: Dan Williams
Cc: akpm, Li Zhijian, linux-cxl, Jonathan.Cameron, mika.westerberg,
ilpo.jarvinen, bhelgaas, huang.ying.caritas
On Tue, Mar 04, 2025 at 06:22:53PM -0800, Dan Williams wrote:
> Li reports a kmemleak detection in get_free_mem_region() error unwind
> path:
>
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
The above lines are not important and may be removed from the commit message.
> It turns out it not only leaks memory, also fails to unwind changes to
> the resource tree (@base, usually iomem_resource).
>
> Fix this by consolidating the devres and devm paths into just devres,
> and move those details to a wrapper function. So now
> __get_free_mem_region() only needs to worry about alloc_resource()
> unwinding, and the devres failure path is resolved before touching the
> resource tree.
...
> static void devm_region_release(struct device *dev, void *res)
> {
> struct region_devres *this = res;
>
> - __release_region(this->parent, this->start, this->n);
> + if (!this->res) {
> + __release_region(this->parent, this->start, this->n);
> + } else {
> + remove_resource(this->res);
> + free_resource(this->res);
> + }
Why not positive conditional?
if (this->res) {
remove_resource(this->res);
free_resource(this->res);
} else {
__release_region(this->parent, this->start, this->n);
}
> }
...
> +static struct resource *
> +get_free_mem_region(struct device *dev, struct resource *base,
> + resource_size_t size, const unsigned long align,
> + const char *name, const unsigned long desc,
> + const unsigned long flags)
> +{
> +
> + struct region_devres *dr __free(kfree) = NULL;
> + struct resource *res;
> +
> + if (dev) {
> + dr = devres_alloc(devm_region_release,
> + sizeof(struct region_devres), GFP_KERNEL);
sizeof(*dr)
> + if (!dr)
> + return ERR_PTR(-ENOMEM);
> + }
> + res = __get_free_mem_region(base, size, align, name, desc, flags);
> + if (IS_ERR(res) || !dr)
> + return res;
But wouldn't be easier to utilise devm_add_action_or_reset()?
> + dr->parent = base;
> + dr->start = res->start;
> + dr->n = resource_size(res);
> +
> + /* See 'struct region_devres' definition for details */
> + if ((flags & GFR_REQUEST_REGION) == 0)
> + dr->res = res;
> +
> + devres_add(dev, no_free_ptr(dr));
> +
> + return res;
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-05 2:22 [PATCH] resource: Fix resource leak in get_free_mem_region() Dan Williams
2025-03-05 2:54 ` Zhijian Li (Fujitsu)
2025-03-05 10:22 ` Andy Shevchenko
@ 2025-03-14 11:49 ` Jonathan Cameron
2025-03-14 14:44 ` Andy Shevchenko
2025-06-04 6:06 ` Zhijian Li (Fujitsu)
3 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2025-03-14 11:49 UTC (permalink / raw)
To: Dan Williams
Cc: akpm, Li Zhijian, linux-cxl, andriy.shevchenko, mika.westerberg,
ilpo.jarvinen, bhelgaas, huang.ying.caritas
On Tue, 04 Mar 2025 18:22:53 -0800
Dan Williams <dan.j.williams@intel.com> wrote:
> Li reports a kmemleak detection in get_free_mem_region() error unwind
> path:
>
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> It turns out it not only leaks memory, also fails to unwind changes to
> the resource tree (@base, usually iomem_resource).
>
> Fix this by consolidating the devres and devm paths into just devres,
> and move those details to a wrapper function. So now
> __get_free_mem_region() only needs to worry about alloc_resource()
> unwinding, and the devres failure path is resolved before touching the
> resource tree.
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>
> Andrew, here is a replacement patch for
> resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It
> addresses missing calls to remove_resource() in addition to
> free_resource() in the error path.
Fiddly code. So feel free to ignore comments below as even
if these help, it's still code that requires more coffee than I have
had today to follow easily.
Jonathan
>
> +static struct resource *
> +get_free_mem_region(struct device *dev, struct resource *base,
> + resource_size_t size, const unsigned long align,
> + const char *name, const unsigned long desc,
> + const unsigned long flags)
> +{
> +
> + struct region_devres *dr __free(kfree) = NULL;
> + struct resource *res;
> +
Avoid split of the constructor / destructor with a ternary though
error check is worse... Hmm.
struct region_devres *dr __free(kfree) =
dev ? devres_alloc(devm_region_release, sizeof(*dr), GFP_KERNEL) : NULL;
if (dev && !dr)
return ERR_PTR(-ENOMEM);
So on balance up to you, but at very least put that __free(kfree) direclty
above the set.
> + if (dev) {
> + dr = devres_alloc(devm_region_release,
> + sizeof(struct region_devres), GFP_KERNEL);
> + if (!dr)
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + res = __get_free_mem_region(base, size, align, name, desc, flags);
> +
Probably no blank line here would be nicer.
> + if (IS_ERR(res) || !dr)
This conflates error and don't care if error cases and briefly
made me scratch my head.
Maybe though it increase indent don't do early
return on the !dr case.
if (IS_ERR(res))
return res;
if (dr) {
dr->parent = base;
...
}
return res;
> + return res;
> +
> + dr->parent = base;
> + dr->start = res->start;
> + dr->n = resource_size(res);
> +
> + /* See 'struct region_devres' definition for details */
> + if ((flags & GFR_REQUEST_REGION) == 0)
> + dr->res = res;
> +
> + devres_add(dev, no_free_ptr(dr));
> +
> + return res;
> +}
> +
> /**
> * devm_request_free_mem_region - find free region for device private memory
> *
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-14 11:49 ` Jonathan Cameron
@ 2025-03-14 14:44 ` Andy Shevchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2025-03-14 14:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dan Williams, akpm, Li Zhijian, linux-cxl, mika.westerberg,
ilpo.jarvinen, bhelgaas, huang.ying.caritas
On Fri, Mar 14, 2025 at 11:49:01AM +0000, Jonathan Cameron wrote:
> On Tue, 04 Mar 2025 18:22:53 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
...
> > + struct region_devres *dr __free(kfree) = NULL;
> > + struct resource *res;
> > +
> Avoid split of the constructor / destructor with a ternary though
> error check is worse... Hmm.
>
> struct region_devres *dr __free(kfree) =
> dev ? devres_alloc(devm_region_release, sizeof(*dr), GFP_KERNEL) : NULL;
> if (dev && !dr)
> return ERR_PTR(-ENOMEM);
>
> So on balance up to you,
I'm against ternary, it looks unreadable in this case.
...
> but at very least put that __free(kfree) direclty
> above the set.
This I am neutral about. Any thing works for me.
The rest you suggested is +1 from me.
> > + if (dev) {
> > + dr = devres_alloc(devm_region_release,
> > + sizeof(struct region_devres), GFP_KERNEL);
> > + if (!dr)
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + res = __get_free_mem_region(base, size, align, name, desc, flags);
> > +
> Probably no blank line here would be nicer.
>
> > + if (IS_ERR(res) || !dr)
>
> This conflates error and don't care if error cases and briefly
> made me scratch my head.
>
> Maybe though it increase indent don't do early
> return on the !dr case.
>
> if (IS_ERR(res))
> return res;
>
> if (dr) {
> dr->parent = base;
>
> ...
>
> }
>
> return res;
>
> > + return res;
> > +
> > + dr->parent = base;
> > + dr->start = res->start;
> > + dr->n = resource_size(res);
> > +
> > + /* See 'struct region_devres' definition for details */
> > + if ((flags & GFR_REQUEST_REGION) == 0)
> > + dr->res = res;
> > +
> > + devres_add(dev, no_free_ptr(dr));
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-05 2:22 [PATCH] resource: Fix resource leak in get_free_mem_region() Dan Williams
` (2 preceding siblings ...)
2025-03-14 11:49 ` Jonathan Cameron
@ 2025-06-04 6:06 ` Zhijian Li (Fujitsu)
2025-06-04 6:31 ` Andrew Morton
3 siblings, 1 reply; 14+ messages in thread
From: Zhijian Li (Fujitsu) @ 2025-06-04 6:06 UTC (permalink / raw)
To: Dan Williams, akpm@linux-foundation.org
Cc: linux-cxl@vger.kernel.org, Jonathan.Cameron@huawei.com,
andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, ilpo.jarvinen@linux.intel.com,
bhelgaas@google.com, huang.ying.caritas@gmail.com
Hello,
Ping, I hit his leak again in v6.15+
I'm surprised this patch has not been merged into upstream.
Thanks
Zhijian
On 05/03/2025 10:22, Dan Williams wrote:
> Li reports a kmemleak detection in get_free_mem_region() error unwind
> path:
>
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> It turns out it not only leaks memory, also fails to unwind changes to
> the resource tree (@base, usually iomem_resource).
>
> Fix this by consolidating the devres and devm paths into just devres,
> and move those details to a wrapper function. So now
> __get_free_mem_region() only needs to worry about alloc_resource()
> unwinding, and the devres failure path is resolved before touching the
> resource tree.
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>
> Andrew, here is a replacement patch for
> resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It
> addresses missing calls to remove_resource() in addition to
> free_resource() in the error path.
>
> kernel/resource.c | 105 ++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 60 insertions(+), 45 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 12004452d999..80d10714cb38 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -172,6 +172,8 @@ static void free_resource(struct resource *res)
> kfree(res);
> }
>
> +DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T))
> +
> static struct resource *alloc_resource(gfp_t flags)
> {
> return kzalloc(sizeof(struct resource), flags);
> @@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new)
> }
> EXPORT_SYMBOL(devm_release_resource);
>
> +/*
> + * The GFR_REQUEST_REGION case performs a request_region() to be paired
> + * with release_region(). The alloc_free_mem_region() path performs
> + * insert_resource() to be paired with {remove,free}_resource(). The
> + * @res member differentiates the 2 cases.
> + */
> struct region_devres {
> struct resource *parent;
> resource_size_t start;
> resource_size_t n;
> + struct resource *res;
> };
>
> static void devm_region_release(struct device *dev, void *res)
> {
> struct region_devres *this = res;
>
> - __release_region(this->parent, this->start, this->n);
> + if (!this->res) {
> + __release_region(this->parent, this->start, this->n);
> + } else {
> + remove_resource(this->res);
> + free_resource(this->res);
> + }
> }
>
> static int devm_region_match(struct device *dev, void *res, void *match_data)
> @@ -1908,43 +1922,19 @@ static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
> return addr + size;
> }
>
> -static void remove_free_mem_region(void *_res)
> -{
> - struct resource *res = _res;
> -
> - if (res->parent)
> - remove_resource(res);
> - free_resource(res);
> -}
> -
> static struct resource *
> -get_free_mem_region(struct device *dev, struct resource *base,
> - resource_size_t size, const unsigned long align,
> - const char *name, const unsigned long desc,
> - const unsigned long flags)
> +__get_free_mem_region(struct resource *base, resource_size_t size,
> + const unsigned long align, const char *name,
> + const unsigned long desc, const unsigned long flags)
> {
> resource_size_t addr;
> - struct resource *res;
> - struct region_devres *dr = NULL;
>
> size = ALIGN(size, align);
>
> - res = alloc_resource(GFP_KERNEL);
> + struct resource *res __free(free_resource) = alloc_resource(GFP_KERNEL);
> if (!res)
> return ERR_PTR(-ENOMEM);
>
> - if (dev && (flags & GFR_REQUEST_REGION)) {
> - dr = devres_alloc(devm_region_release,
> - sizeof(struct region_devres), GFP_KERNEL);
> - if (!dr) {
> - free_resource(res);
> - return ERR_PTR(-ENOMEM);
> - }
> - } else if (dev) {
> - if (devm_add_action_or_reset(dev, remove_free_mem_region, res))
> - return ERR_PTR(-ENOMEM);
> - }
> -
> write_lock(&resource_lock);
> for (addr = gfr_start(base, size, align, flags);
> gfr_continue(base, addr, align, flags);
> @@ -1958,17 +1948,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
> size, name, 0))
> break;
>
> - if (dev) {
> - dr->parent = &iomem_resource;
> - dr->start = addr;
> - dr->n = size;
> - devres_add(dev, dr);
> - }
> -
> res->desc = desc;
> write_unlock(&resource_lock);
>
> -
> /*
> * A driver is claiming this region so revoke any
> * mappings.
> @@ -1985,25 +1967,58 @@ get_free_mem_region(struct device *dev, struct resource *base,
> * Only succeed if the resource hosts an exclusive
> * range after the insert
> */
> - if (__insert_resource(base, res) || res->child)
> + if (__insert_resource(base, res))
> + break;
> + if (res->child) {
> + remove_resource(res);
> break;
> + }
>
> write_unlock(&resource_lock);
> }
>
> - return res;
> + return no_free_ptr(res);
> }
> write_unlock(&resource_lock);
>
> - if (flags & GFR_REQUEST_REGION) {
> - free_resource(res);
> - devres_free(dr);
> - } else if (dev)
> - devm_release_action(dev, remove_free_mem_region, res);
> -
> return ERR_PTR(-ERANGE);
> }
>
> +static struct resource *
> +get_free_mem_region(struct device *dev, struct resource *base,
> + resource_size_t size, const unsigned long align,
> + const char *name, const unsigned long desc,
> + const unsigned long flags)
> +{
> +
> + struct region_devres *dr __free(kfree) = NULL;
> + struct resource *res;
> +
> + if (dev) {
> + dr = devres_alloc(devm_region_release,
> + sizeof(struct region_devres), GFP_KERNEL);
> + if (!dr)
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + res = __get_free_mem_region(base, size, align, name, desc, flags);
> +
> + if (IS_ERR(res) || !dr)
> + return res;
> +
> + dr->parent = base;
> + dr->start = res->start;
> + dr->n = resource_size(res);
> +
> + /* See 'struct region_devres' definition for details */
> + if ((flags & GFR_REQUEST_REGION) == 0)
> + dr->res = res;
> +
> + devres_add(dev, no_free_ptr(dr));
> +
> + return res;
> +}
> +
> /**
> * devm_request_free_mem_region - find free region for device private memory
> *
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-06-04 6:06 ` Zhijian Li (Fujitsu)
@ 2025-06-04 6:31 ` Andrew Morton
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Morton @ 2025-06-04 6:31 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: Dan Williams, linux-cxl@vger.kernel.org,
Jonathan.Cameron@huawei.com, andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, ilpo.jarvinen@linux.intel.com,
bhelgaas@google.com, huang.ying.caritas@gmail.com
On Wed, 4 Jun 2025 06:06:48 +0000 "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:
> Hello,
>
> Ping, I hit his leak again in v6.15+
>
> I'm surprised this patch has not been merged into upstream.
I'm struggling because I'm not subscribed to linux-cxl (please cc
linux-kernel on patches) but it appears that Dan's alternative patch is
due for an update (thanks to comments from Andy and Jonathan) and that
appears to have not happened.
So Dan, can you please take a look, see if we can get this finished?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-04 4:34 Li Zhijian
@ 2025-03-04 9:22 ` Mika Westerberg
2025-03-04 9:44 ` Zhijian Li (Fujitsu)
2025-03-04 12:09 ` Andy Shevchenko
2025-03-04 23:43 ` Dan Williams
2 siblings, 1 reply; 14+ messages in thread
From: Mika Westerberg @ 2025-03-04 9:22 UTC (permalink / raw)
To: Li Zhijian
Cc: linux-kernel, Andrew Morton, Andy Shevchenko, ilpo.jarvinen,
Bjorn Helgaas, Ying Huang, Dan Williams, Jonathan Cameron,
linux-cxl
On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote:
> The leak is detected by the kernel memory leak detector (`kmemleak`)
> following a `cxl create-region` failure:
>
> cxl_acpi ACPI0017:00: decoder0.0: created region2
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> kernel/resource.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 12004452d999..aa0b1da143eb 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
> devres_free(dr);
> } else if (dev)
> devm_release_action(dev, remove_free_mem_region, res);
> + else
> + free_resource(res);
It should use {} as per coding style:
} else {
free_resource(res);
}
(probably should add that to the previous branch too).
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-04 9:22 ` Mika Westerberg
@ 2025-03-04 9:44 ` Zhijian Li (Fujitsu)
2025-03-04 10:03 ` Mika Westerberg
0 siblings, 1 reply; 14+ messages in thread
From: Zhijian Li (Fujitsu) @ 2025-03-04 9:44 UTC (permalink / raw)
To: Mika Westerberg
Cc: linux-kernel@vger.kernel.org, Andrew Morton, Andy Shevchenko,
ilpo.jarvinen@linux.intel.com, Bjorn Helgaas, Ying Huang,
Dan Williams, Jonathan Cameron, linux-cxl@vger.kernel.org
On 04/03/2025 17:22, Mika Westerberg wrote:
> On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote:
>> The leak is detected by the kernel memory leak detector (`kmemleak`)
>> following a `cxl create-region` failure:
>>
>> cxl_acpi ACPI0017:00: decoder0.0: created region2
>> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
>> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>>
>> __kmalloc_cache_noprof+0x28c/0x350
>> get_free_mem_region+0x45/0x380
>> alloc_free_mem_region+0x1d/0x30
>> size_store+0x180/0x290 [cxl_core]
>> kernfs_fop_write_iter+0x13f/0x1e0
>> vfs_write+0x37c/0x540
>> ksys_write+0x68/0xe0
>> do_syscall_64+0x6e/0x190
>> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> kernel/resource.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 12004452d999..aa0b1da143eb 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
>> devres_free(dr);
>> } else if (dev)
>> devm_release_action(dev, remove_free_mem_region, res);
>> + else
>> + free_resource(res);
>
> It should use {} as per coding style:
The script/checkpatch.pl is happy with both of these 2 styles in practice.
Thanks
Zhijian
>
> } else {
> free_resource(res);
> }
>
> (probably should add that to the previous branch too).
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-04 9:44 ` Zhijian Li (Fujitsu)
@ 2025-03-04 10:03 ` Mika Westerberg
0 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2025-03-04 10:03 UTC (permalink / raw)
To: Zhijian Li (Fujitsu)
Cc: linux-kernel@vger.kernel.org, Andrew Morton, Andy Shevchenko,
ilpo.jarvinen@linux.intel.com, Bjorn Helgaas, Ying Huang,
Dan Williams, Jonathan Cameron, linux-cxl@vger.kernel.org
On Tue, Mar 04, 2025 at 09:44:38AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 04/03/2025 17:22, Mika Westerberg wrote:
> > On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote:
> >> The leak is detected by the kernel memory leak detector (`kmemleak`)
> >> following a `cxl create-region` failure:
> >>
> >> cxl_acpi ACPI0017:00: decoder0.0: created region2
> >> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> >> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
> >>
> >> __kmalloc_cache_noprof+0x28c/0x350
> >> get_free_mem_region+0x45/0x380
> >> alloc_free_mem_region+0x1d/0x30
> >> size_store+0x180/0x290 [cxl_core]
> >> kernfs_fop_write_iter+0x13f/0x1e0
> >> vfs_write+0x37c/0x540
> >> ksys_write+0x68/0xe0
> >> do_syscall_64+0x6e/0x190
> >> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >>
> >> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >> ---
> >> kernel/resource.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/kernel/resource.c b/kernel/resource.c
> >> index 12004452d999..aa0b1da143eb 100644
> >> --- a/kernel/resource.c
> >> +++ b/kernel/resource.c
> >> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
> >> devres_free(dr);
> >> } else if (dev)
> >> devm_release_action(dev, remove_free_mem_region, res);
> >> + else
> >> + free_resource(res);
> >
> > It should use {} as per coding style:
>
>
> The script/checkpatch.pl is happy with both of these 2 styles in practice.
It is but the coding style prefers then use {} around all branches, see:
https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
} else if (dev) {
devm_release_action(dev, remove_free_mem_region, res);
} else {
free_resource(res);
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-04 4:34 Li Zhijian
2025-03-04 9:22 ` Mika Westerberg
@ 2025-03-04 12:09 ` Andy Shevchenko
2025-03-04 23:43 ` Dan Williams
2 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2025-03-04 12:09 UTC (permalink / raw)
To: Li Zhijian
Cc: linux-kernel, Andrew Morton, ilpo.jarvinen, Mika Westerberg,
Bjorn Helgaas, Ying Huang, Dan Williams, Jonathan Cameron,
linux-cxl
On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote:
> The leak is detected by the kernel memory leak detector (`kmemleak`)
> following a `cxl create-region` failure:
>
> cxl_acpi ACPI0017:00: decoder0.0: created region2
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
The below lines have no importance to be present in the commit message.
Please read Submitting Patches documentation that justifies my comment.
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] resource: Fix resource leak in get_free_mem_region()
2025-03-04 4:34 Li Zhijian
2025-03-04 9:22 ` Mika Westerberg
2025-03-04 12:09 ` Andy Shevchenko
@ 2025-03-04 23:43 ` Dan Williams
2 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2025-03-04 23:43 UTC (permalink / raw)
To: Li Zhijian, linux-kernel
Cc: Andrew Morton, Andy Shevchenko, ilpo.jarvinen, Mika Westerberg,
Bjorn Helgaas, Ying Huang, Dan Williams, Jonathan Cameron,
linux-cxl, Li Zhijian
Li Zhijian wrote:
> The leak is detected by the kernel memory leak detector (`kmemleak`)
> following a `cxl create-region` failure:
>
> cxl_acpi ACPI0017:00: decoder0.0: created region2
> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> __kmalloc_cache_noprof+0x28c/0x350
> get_free_mem_region+0x45/0x380
> alloc_free_mem_region+0x1d/0x30
> size_store+0x180/0x290 [cxl_core]
> kernfs_fop_write_iter+0x13f/0x1e0
> vfs_write+0x37c/0x540
> ksys_write+0x68/0xe0
> do_syscall_64+0x6e/0x190
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> kernel/resource.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 12004452d999..aa0b1da143eb 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
> devres_free(dr);
> } else if (dev)
> devm_release_action(dev, remove_free_mem_region, res);
> + else
> + free_resource(res);
This looks deceptively correct, but if the __insert_resource() call
succeeded above then this needs to optionally be paired with
remove_resource().
I think this function needs a rethink because mixing the devres, devm,
and alloc_resource() failure cases makes mistakes like this hard to see.
Here is a replacement proposal, only compile-tested:
-- >8 --
From a01a28304e547da1f6287eecf3aeb0ebc6f48e2b Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Tue, 4 Mar 2025 15:12:19 -0800
Subject: [PATCH] resource: Fix resource leak in get_free_mem_region()
Li reports a kmemleak detection in get_free_mem_region() error unwind
path:
cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200]
kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
__kmalloc_cache_noprof+0x28c/0x350
get_free_mem_region+0x45/0x380
alloc_free_mem_region+0x1d/0x30
size_store+0x180/0x290 [cxl_core]
kernfs_fop_write_iter+0x13f/0x1e0
vfs_write+0x37c/0x540
ksys_write+0x68/0xe0
do_syscall_64+0x6e/0x190
entry_SYSCALL_64_after_hwframe+0x76/0x7e
It turns out it not only leaks memory, also fails to unwind changes to
the resource tree (@base, usually iomem_resource).
Fix this by consolidating the devres and devm paths into just devres,
and move those details to a wrapper function. So now
__get_free_mem_region() only needs to worry about alloc_resource()
unwinding, and the devres failure path is resolved before touching the
resource tree.
Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()")
Reported-by: Li Zhijian <lizhijian@fujitsu.com>
Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
kernel/resource.c | 105 ++++++++++++++++++++++++++--------------------
1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 12004452d999..80d10714cb38 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -172,6 +172,8 @@ static void free_resource(struct resource *res)
kfree(res);
}
+DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T))
+
static struct resource *alloc_resource(gfp_t flags)
{
return kzalloc(sizeof(struct resource), flags);
@@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new)
}
EXPORT_SYMBOL(devm_release_resource);
+/*
+ * The GFR_REQUEST_REGION case performs a request_region() to be paired
+ * with release_region(). The alloc_free_mem_region() path performs
+ * insert_resource() to be paired with {remove,free}_resource(). The
+ * @res member differentiates the 2 cases.
+ */
struct region_devres {
struct resource *parent;
resource_size_t start;
resource_size_t n;
+ struct resource *res;
};
static void devm_region_release(struct device *dev, void *res)
{
struct region_devres *this = res;
- __release_region(this->parent, this->start, this->n);
+ if (!this->res) {
+ __release_region(this->parent, this->start, this->n);
+ } else {
+ remove_resource(this->res);
+ free_resource(this->res);
+ }
}
static int devm_region_match(struct device *dev, void *res, void *match_data)
@@ -1908,43 +1922,19 @@ static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
return addr + size;
}
-static void remove_free_mem_region(void *_res)
-{
- struct resource *res = _res;
-
- if (res->parent)
- remove_resource(res);
- free_resource(res);
-}
-
static struct resource *
-get_free_mem_region(struct device *dev, struct resource *base,
- resource_size_t size, const unsigned long align,
- const char *name, const unsigned long desc,
- const unsigned long flags)
+__get_free_mem_region(struct resource *base, resource_size_t size,
+ const unsigned long align, const char *name,
+ const unsigned long desc, const unsigned long flags)
{
resource_size_t addr;
- struct resource *res;
- struct region_devres *dr = NULL;
size = ALIGN(size, align);
- res = alloc_resource(GFP_KERNEL);
+ struct resource *res __free(free_resource) = alloc_resource(GFP_KERNEL);
if (!res)
return ERR_PTR(-ENOMEM);
- if (dev && (flags & GFR_REQUEST_REGION)) {
- dr = devres_alloc(devm_region_release,
- sizeof(struct region_devres), GFP_KERNEL);
- if (!dr) {
- free_resource(res);
- return ERR_PTR(-ENOMEM);
- }
- } else if (dev) {
- if (devm_add_action_or_reset(dev, remove_free_mem_region, res))
- return ERR_PTR(-ENOMEM);
- }
-
write_lock(&resource_lock);
for (addr = gfr_start(base, size, align, flags);
gfr_continue(base, addr, align, flags);
@@ -1958,17 +1948,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
size, name, 0))
break;
- if (dev) {
- dr->parent = &iomem_resource;
- dr->start = addr;
- dr->n = size;
- devres_add(dev, dr);
- }
-
res->desc = desc;
write_unlock(&resource_lock);
-
/*
* A driver is claiming this region so revoke any
* mappings.
@@ -1985,25 +1967,58 @@ get_free_mem_region(struct device *dev, struct resource *base,
* Only succeed if the resource hosts an exclusive
* range after the insert
*/
- if (__insert_resource(base, res) || res->child)
+ if (__insert_resource(base, res))
+ break;
+ if (res->child) {
+ remove_resource(res);
break;
+ }
write_unlock(&resource_lock);
}
- return res;
+ return no_free_ptr(res);
}
write_unlock(&resource_lock);
- if (flags & GFR_REQUEST_REGION) {
- free_resource(res);
- devres_free(dr);
- } else if (dev)
- devm_release_action(dev, remove_free_mem_region, res);
-
return ERR_PTR(-ERANGE);
}
+static struct resource *
+get_free_mem_region(struct device *dev, struct resource *base,
+ resource_size_t size, const unsigned long align,
+ const char *name, const unsigned long desc,
+ const unsigned long flags)
+{
+
+ struct region_devres *dr __free(kfree) = NULL;
+ struct resource *res;
+
+ if (dev) {
+ dr = devres_alloc(devm_region_release,
+ sizeof(struct region_devres), GFP_KERNEL);
+ if (!dr)
+ return ERR_PTR(-ENOMEM);
+ }
+
+ res = __get_free_mem_region(base, size, align, name, desc, flags);
+
+ if (IS_ERR(res) || !dr)
+ return res;
+
+ dr->parent = base;
+ dr->start = res->start;
+ dr->n = resource_size(res);
+
+ /* See 'struct region_devres' definition for details */
+ if ((flags & GFR_REQUEST_REGION) == 0)
+ dr->res = res;
+
+ devres_add(dev, no_free_ptr(dr));
+
+ return res;
+}
+
/**
* devm_request_free_mem_region - find free region for device private memory
*
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread