[PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
explicit kfree() in the free path to match its list-managed life time.
Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
on failed lookups. Add NULL guards instratix10_svc_free_memory().

Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
 drivers/firmware/stratix10-svc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 1ef65bf845fc..3b0e2b14180f 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
 	struct gen_pool *genpool = chan->ctrl->genpool;
 	size_t s = roundup(size, 1 << genpool->min_alloc_order);
 
-	pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
+	pmem = kzalloc_obj(*pmem);
 	if (!pmem)
 		return ERR_PTR(-ENOMEM);
 
 	guard(mutex)(&svc_mem_lock);
 	va = gen_pool_alloc(genpool, s);
-	if (!va)
+	if (!va) {
+		kfree(pmem);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	memset((void *)va, 0, s);
 	pa = gen_pool_virt_to_phys(genpool, va);
@@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
 void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
 {
 	struct stratix10_svc_data_mem *pmem;
+
+	if (!chan || !kaddr)
+		return;
 	guard(mutex)(&svc_mem_lock);
 
 	list_for_each_entry(pmem, &svc_data_mem, node)
@@ -1953,10 +1958,9 @@ void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
 				       (unsigned long)kaddr, pmem->size);
 			pmem->vaddr = NULL;
 			list_del(&pmem->node);
+			kfree(pmem);
 			return;
 		}
-
-	list_del(&svc_data_mem);
 }
 EXPORT_SYMBOL_GPL(stratix10_svc_free_memory);
 
-- 
2.43.7

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[Greg Kroah-Hartman]

On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
> From: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
> explicit kfree() in the free path to match its list-managed life time.
> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
> on failed lookups. Add NULL guards instratix10_svc_free_memory().
> 
> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
> 
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---
>  drivers/firmware/stratix10-svc.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index 1ef65bf845fc..3b0e2b14180f 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>  	struct gen_pool *genpool = chan->ctrl->genpool;
>  	size_t s = roundup(size, 1 << genpool->min_alloc_order);
>  
> -	pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
> +	pmem = kzalloc_obj(*pmem);
>  	if (!pmem)
>  		return ERR_PTR(-ENOMEM);
>  
>  	guard(mutex)(&svc_mem_lock);
>  	va = gen_pool_alloc(genpool, s);
> -	if (!va)
> +	if (!va) {
> +		kfree(pmem);
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	memset((void *)va, 0, s);
>  	pa = gen_pool_virt_to_phys(genpool, va);
> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>  void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>  {
>  	struct stratix10_svc_data_mem *pmem;
> +
> +	if (!chan || !kaddr)
> +		return;
>  	guard(mutex)(&svc_mem_lock);
>  
>  	list_for_each_entry(pmem, &svc_data_mem, node)
> @@ -1953,10 +1958,9 @@ void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>  				       (unsigned long)kaddr, pmem->size);
>  			pmem->vaddr = NULL;
>  			list_del(&pmem->node);
> +			kfree(pmem);
>  			return;
>  		}
> -
> -	list_del(&svc_data_mem);
>  }
>  EXPORT_SYMBOL_GPL(stratix10_svc_free_memory);
>  
> -- 
> 2.43.7
> 


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[Greg Kroah-Hartman]

On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
> From: Tze Yee Ng <tze.yee.ng@altera.com>
> 
> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
> explicit kfree() in the free path to match its list-managed life time.
> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
> on failed lookups. Add NULL guards instratix10_svc_free_memory().
> 
> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
> 
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---
>  drivers/firmware/stratix10-svc.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index 1ef65bf845fc..3b0e2b14180f 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>  	struct gen_pool *genpool = chan->ctrl->genpool;
>  	size_t s = roundup(size, 1 << genpool->min_alloc_order);
>  
> -	pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
> +	pmem = kzalloc_obj(*pmem);
>  	if (!pmem)
>  		return ERR_PTR(-ENOMEM);
>  
>  	guard(mutex)(&svc_mem_lock);
>  	va = gen_pool_alloc(genpool, s);
> -	if (!va)
> +	if (!va) {
> +		kfree(pmem);
>  		return ERR_PTR(-ENOMEM);
> +	}
>  
>  	memset((void *)va, 0, s);
>  	pa = gen_pool_virt_to_phys(genpool, va);
> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>  void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>  {
>  	struct stratix10_svc_data_mem *pmem;
> +
> +	if (!chan || !kaddr)
> +		return;

What if one is not NULL but the other is?  Will you not leak memory here
now?

thanks,

greg k-h

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[NG, TZE YEE]

On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
> [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>
>> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
>> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
>> explicit kfree() in the free path to match its list-managed life time.
>> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
>> on failed lookups. Add NULL guards instratix10_svc_free_memory().
>>
>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
>>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
>> ---
>>   drivers/firmware/stratix10-svc.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>> index 1ef65bf845fc..3b0e2b14180f 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
>> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>>        struct gen_pool *genpool = chan->ctrl->genpool;
>>        size_t s = roundup(size, 1 << genpool->min_alloc_order);
>>
>> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
>> +     pmem = kzalloc_obj(*pmem);
>>        if (!pmem)
>>                return ERR_PTR(-ENOMEM);
>>
>>        guard(mutex)(&svc_mem_lock);
>>        va = gen_pool_alloc(genpool, s);
>> -     if (!va)
>> +     if (!va) {
>> +             kfree(pmem);
>>                return ERR_PTR(-ENOMEM);
>> +     }
>>
>>        memset((void *)va, 0, s);
>>        pa = gen_pool_virt_to_phys(genpool, va);
>> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>>   void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>>   {
>>        struct stratix10_svc_data_mem *pmem;
>> +
>> +     if (!chan || !kaddr)
>> +             return;
> 
> What if one is not NULL but the other is?  Will you not leak memory here
> now?
> 
> thanks,
> 
> greg k-h
Hi Greg,

Good catch on the asymmetric case. The guard is not meant to support 
callers passing one NULL and one non-NULL argument.

kaddr == NULL: no-op, nothing to free. The guard prevents the old bug 
where a failed lookup fell through to list_del(&svc_data_mem) and 
corrupted the list head.

chan == NULL with valid kaddr: would leak, but that is invalid API 
usage. The previous code would oops on chan->ctrl->genpool instead of 
freeing. In-tree callers always pass both valid pointers from 
stratix10_svc_allocate_memory().

If you prefer not to silently swallow misuse, I can change this to 
WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check 
and only guard !kaddr so a NULL channel still faults on dereference per 
normal kernel API expectations.

Regards,
Tze Yee

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[Greg Kroah-Hartman]

On Wed, Jun 10, 2026 at 01:47:44AM +0000, NG, TZE YEE wrote:
> On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
> > [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
> >> From: Tze Yee Ng <tze.yee.ng@altera.com>
> >>
> >> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
> >> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
> >> explicit kfree() in the free path to match its list-managed life time.
> >> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
> >> on failed lookups. Add NULL guards instratix10_svc_free_memory().
> >>
> >> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
> >>
> >> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> >> ---
> >>   drivers/firmware/stratix10-svc.c | 12 ++++++++----
> >>   1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> >> index 1ef65bf845fc..3b0e2b14180f 100644
> >> --- a/drivers/firmware/stratix10-svc.c
> >> +++ b/drivers/firmware/stratix10-svc.c
> >> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
> >>        struct gen_pool *genpool = chan->ctrl->genpool;
> >>        size_t s = roundup(size, 1 << genpool->min_alloc_order);
> >>
> >> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
> >> +     pmem = kzalloc_obj(*pmem);
> >>        if (!pmem)
> >>                return ERR_PTR(-ENOMEM);
> >>
> >>        guard(mutex)(&svc_mem_lock);
> >>        va = gen_pool_alloc(genpool, s);
> >> -     if (!va)
> >> +     if (!va) {
> >> +             kfree(pmem);
> >>                return ERR_PTR(-ENOMEM);
> >> +     }
> >>
> >>        memset((void *)va, 0, s);
> >>        pa = gen_pool_virt_to_phys(genpool, va);
> >> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
> >>   void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
> >>   {
> >>        struct stratix10_svc_data_mem *pmem;
> >> +
> >> +     if (!chan || !kaddr)
> >> +             return;
> > 
> > What if one is not NULL but the other is?  Will you not leak memory here
> > now?
> > 
> > thanks,
> > 
> > greg k-h
> Hi Greg,
> 
> Good catch on the asymmetric case. The guard is not meant to support 
> callers passing one NULL and one non-NULL argument.
> 
> kaddr == NULL: no-op, nothing to free. The guard prevents the old bug 
> where a failed lookup fell through to list_del(&svc_data_mem) and 
> corrupted the list head.
> 
> chan == NULL with valid kaddr: would leak, but that is invalid API 
> usage. The previous code would oops on chan->ctrl->genpool instead of 
> freeing. In-tree callers always pass both valid pointers from 
> stratix10_svc_allocate_memory().
> 
> If you prefer not to silently swallow misuse, I can change this to 
> WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check 
> and only guard !kaddr so a NULL channel still faults on dereference per 
> normal kernel API expectations.

WARN_ON() will panic a box if it ever triggers, loosing all data, so
please do not do that.  If this is something that can happen, handle it
properly, don't just crash.

thanks,

greg k-h

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[NG, TZE YEE]

On 10/6/2026 2:07 pm, Greg Kroah-Hartman wrote:
> On Wed, Jun 10, 2026 at 01:47:44AM +0000, NG, TZE YEE wrote:
>> On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
>>> [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
>>>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>>>
>>>> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
>>>> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
>>>> explicit kfree() in the free path to match its list-managed life time.
>>>> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
>>>> on failed lookups. Add NULL guards instratix10_svc_free_memory().
>>>>
>>>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
>>>>
>>>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
>>>> ---
>>>>    drivers/firmware/stratix10-svc.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>>>> index 1ef65bf845fc..3b0e2b14180f 100644
>>>> --- a/drivers/firmware/stratix10-svc.c
>>>> +++ b/drivers/firmware/stratix10-svc.c
>>>> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>>>>         struct gen_pool *genpool = chan->ctrl->genpool;
>>>>         size_t s = roundup(size, 1 << genpool->min_alloc_order);
>>>>
>>>> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
>>>> +     pmem = kzalloc_obj(*pmem);
>>>>         if (!pmem)
>>>>                 return ERR_PTR(-ENOMEM);
>>>>
>>>>         guard(mutex)(&svc_mem_lock);
>>>>         va = gen_pool_alloc(genpool, s);
>>>> -     if (!va)
>>>> +     if (!va) {
>>>> +             kfree(pmem);
>>>>                 return ERR_PTR(-ENOMEM);
>>>> +     }
>>>>
>>>>         memset((void *)va, 0, s);
>>>>         pa = gen_pool_virt_to_phys(genpool, va);
>>>> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>>>>    void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>>>>    {
>>>>         struct stratix10_svc_data_mem *pmem;
>>>> +
>>>> +     if (!chan || !kaddr)
>>>> +             return;
>>>
>>> What if one is not NULL but the other is?  Will you not leak memory here
>>> now?
>>>
>>> thanks,
>>>
>>> greg k-h
>> Hi Greg,
>>
>> Good catch on the asymmetric case. The guard is not meant to support
>> callers passing one NULL and one non-NULL argument.
>>
>> kaddr == NULL: no-op, nothing to free. The guard prevents the old bug
>> where a failed lookup fell through to list_del(&svc_data_mem) and
>> corrupted the list head.
>>
>> chan == NULL with valid kaddr: would leak, but that is invalid API
>> usage. The previous code would oops on chan->ctrl->genpool instead of
>> freeing. In-tree callers always pass both valid pointers from
>> stratix10_svc_allocate_memory().
>>
>> If you prefer not to silently swallow misuse, I can change this to
>> WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check
>> and only guard !kaddr so a NULL channel still faults on dereference per
>> normal kernel API expectations.
> 
> WARN_ON() will panic a box if it ever triggers, loosing all data, so
> please do not do that.  If this is something that can happen, handle it
> properly, don't just crash.
> 
> thanks,
> 
> greg k-h

Hi Greg,

Thanks for the review.

I went through the code again. There are already checks for both kaddr 
and chan to guard against invalid API use. The !chan check is mainly a 
failsafe in case a caller misuses the API.

Regarding WARN_ON(), thanks for catching that — I had added it to flag 
the !chan case. Would it be acceptable if I split the checks into two 
conditions and only warn on !chan?

if (!chan) {
	WARN_ON_ONCE(!chan);
	return;
}

if (!kaddr)
	return;


Thanks,
Tze Yee

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[Greg Kroah-Hartman]

On Fri, Jun 12, 2026 at 03:43:44AM +0000, NG, TZE YEE wrote:
> On 10/6/2026 2:07 pm, Greg Kroah-Hartman wrote:
> > On Wed, Jun 10, 2026 at 01:47:44AM +0000, NG, TZE YEE wrote:
> >> On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
> >>> [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>
> >>> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
> >>>> From: Tze Yee Ng <tze.yee.ng@altera.com>
> >>>>
> >>>> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
> >>>> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
> >>>> explicit kfree() in the free path to match its list-managed life time.
> >>>> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
> >>>> on failed lookups. Add NULL guards instratix10_svc_free_memory().
> >>>>
> >>>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
> >>>>
> >>>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> >>>> ---
> >>>>    drivers/firmware/stratix10-svc.c | 12 ++++++++----
> >>>>    1 file changed, 8 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> >>>> index 1ef65bf845fc..3b0e2b14180f 100644
> >>>> --- a/drivers/firmware/stratix10-svc.c
> >>>> +++ b/drivers/firmware/stratix10-svc.c
> >>>> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
> >>>>         struct gen_pool *genpool = chan->ctrl->genpool;
> >>>>         size_t s = roundup(size, 1 << genpool->min_alloc_order);
> >>>>
> >>>> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
> >>>> +     pmem = kzalloc_obj(*pmem);
> >>>>         if (!pmem)
> >>>>                 return ERR_PTR(-ENOMEM);
> >>>>
> >>>>         guard(mutex)(&svc_mem_lock);
> >>>>         va = gen_pool_alloc(genpool, s);
> >>>> -     if (!va)
> >>>> +     if (!va) {
> >>>> +             kfree(pmem);
> >>>>                 return ERR_PTR(-ENOMEM);
> >>>> +     }
> >>>>
> >>>>         memset((void *)va, 0, s);
> >>>>         pa = gen_pool_virt_to_phys(genpool, va);
> >>>> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
> >>>>    void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
> >>>>    {
> >>>>         struct stratix10_svc_data_mem *pmem;
> >>>> +
> >>>> +     if (!chan || !kaddr)
> >>>> +             return;
> >>>
> >>> What if one is not NULL but the other is?  Will you not leak memory here
> >>> now?
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >> Hi Greg,
> >>
> >> Good catch on the asymmetric case. The guard is not meant to support
> >> callers passing one NULL and one non-NULL argument.
> >>
> >> kaddr == NULL: no-op, nothing to free. The guard prevents the old bug
> >> where a failed lookup fell through to list_del(&svc_data_mem) and
> >> corrupted the list head.
> >>
> >> chan == NULL with valid kaddr: would leak, but that is invalid API
> >> usage. The previous code would oops on chan->ctrl->genpool instead of
> >> freeing. In-tree callers always pass both valid pointers from
> >> stratix10_svc_allocate_memory().
> >>
> >> If you prefer not to silently swallow misuse, I can change this to
> >> WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check
> >> and only guard !kaddr so a NULL channel still faults on dereference per
> >> normal kernel API expectations.
> > 
> > WARN_ON() will panic a box if it ever triggers, loosing all data, so
> > please do not do that.  If this is something that can happen, handle it
> > properly, don't just crash.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Thanks for the review.
> 
> I went through the code again. There are already checks for both kaddr 
> and chan to guard against invalid API use. The !chan check is mainly a 
> failsafe in case a caller misuses the API.
> 
> Regarding WARN_ON(), thanks for catching that — I had added it to flag 
> the !chan case. Would it be acceptable if I split the checks into two 
> conditions and only warn on !chan?
> 
> if (!chan) {
> 	WARN_ON_ONCE(!chan);

again, never add new WARN_ON calls for something that can actually
happen.  If it can never happen, there is no need to check for it.

thanks,

greg k-h

Re: [PATCH] firmware: stratix10-svc: fix memory leaks and list corruption bugs

[NG, TZE YEE]

On 12/6/2026 3:54 pm, Greg Kroah-Hartman wrote:
> On Fri, Jun 12, 2026 at 03:43:44AM +0000, NG, TZE YEE wrote:
>> On 10/6/2026 2:07 pm, Greg Kroah-Hartman wrote:
>>> On Wed, Jun 10, 2026 at 01:47:44AM +0000, NG, TZE YEE wrote:
>>>> On 9/6/2026 6:13 pm, Greg Kroah-Hartman wrote:
>>>>> [Some people who received this message don't often get email from gregkh@linuxfoundation.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> On Tue, Jun 09, 2026 at 02:19:44AM -0700, tze.yee.ng@altera.com wrote:
>>>>>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>>>>>
>>>>>> Fix a memory leak when gen_pool_alloc() fails by freeing pmem on the error
>>>>>> path. Switch pmem allocation from devm_kzalloc() to kzalloc() with
>>>>>> explicit kfree() in the free path to match its list-managed life time.
>>>>>> Remove the erroneous list_del(&svc_data_mem) which corrupted the list head
>>>>>> on failed lookups. Add NULL guards instratix10_svc_free_memory().
>>>>>>
>>>>>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer driver")
>>>>>>
>>>>>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
>>>>>> ---
>>>>>>     drivers/firmware/stratix10-svc.c | 12 ++++++++----
>>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>>>>>> index 1ef65bf845fc..3b0e2b14180f 100644
>>>>>> --- a/drivers/firmware/stratix10-svc.c
>>>>>> +++ b/drivers/firmware/stratix10-svc.c
>>>>>> @@ -1912,14 +1912,16 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
>>>>>>          struct gen_pool *genpool = chan->ctrl->genpool;
>>>>>>          size_t s = roundup(size, 1 << genpool->min_alloc_order);
>>>>>>
>>>>>> -     pmem = devm_kzalloc(chan->ctrl->dev, sizeof(*pmem), GFP_KERNEL);
>>>>>> +     pmem = kzalloc_obj(*pmem);
>>>>>>          if (!pmem)
>>>>>>                  return ERR_PTR(-ENOMEM);
>>>>>>
>>>>>>          guard(mutex)(&svc_mem_lock);
>>>>>>          va = gen_pool_alloc(genpool, s);
>>>>>> -     if (!va)
>>>>>> +     if (!va) {
>>>>>> +             kfree(pmem);
>>>>>>                  return ERR_PTR(-ENOMEM);
>>>>>> +     }
>>>>>>
>>>>>>          memset((void *)va, 0, s);
>>>>>>          pa = gen_pool_virt_to_phys(genpool, va);
>>>>>> @@ -1945,6 +1947,9 @@ EXPORT_SYMBOL_GPL(stratix10_svc_allocate_memory);
>>>>>>     void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
>>>>>>     {
>>>>>>          struct stratix10_svc_data_mem *pmem;
>>>>>> +
>>>>>> +     if (!chan || !kaddr)
>>>>>> +             return;
>>>>>
>>>>> What if one is not NULL but the other is?  Will you not leak memory here
>>>>> now?
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>> Hi Greg,
>>>>
>>>> Good catch on the asymmetric case. The guard is not meant to support
>>>> callers passing one NULL and one non-NULL argument.
>>>>
>>>> kaddr == NULL: no-op, nothing to free. The guard prevents the old bug
>>>> where a failed lookup fell through to list_del(&svc_data_mem) and
>>>> corrupted the list head.
>>>>
>>>> chan == NULL with valid kaddr: would leak, but that is invalid API
>>>> usage. The previous code would oops on chan->ctrl->genpool instead of
>>>> freeing. In-tree callers always pass both valid pointers from
>>>> stratix10_svc_allocate_memory().
>>>>
>>>> If you prefer not to silently swallow misuse, I can change this to
>>>> WARN_ON_ONCE(!chan || !kaddr) before returning, or drop the !chan check
>>>> and only guard !kaddr so a NULL channel still faults on dereference per
>>>> normal kernel API expectations.
>>>
>>> WARN_ON() will panic a box if it ever triggers, loosing all data, so
>>> please do not do that.  If this is something that can happen, handle it
>>> properly, don't just crash.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Hi Greg,
>>
>> Thanks for the review.
>>
>> I went through the code again. There are already checks for both kaddr
>> and chan to guard against invalid API use. The !chan check is mainly a
>> failsafe in case a caller misuses the API.
>>
>> Regarding WARN_ON(), thanks for catching that — I had added it to flag
>> the !chan case. Would it be acceptable if I split the checks into two
>> conditions and only warn on !chan?
>>
>> if (!chan) {
>> 	WARN_ON_ONCE(!chan);
> 
> again, never add new WARN_ON calls for something that can actually
> happen.  If it can never happen, there is no need to check for it.
> 
> thanks,
> 
> greg k-h
Hi Greg,

Alright, I will remove the WARN_ON calls. Also, I will remove the NULL 
checks from stratix10_svc_free_memory(). If a NULL is passed into a 
required API parameter the kernel crash immediately, so the bug is 
caught and fixed during development.

I will resend v2 with the fixes above and the cc: stable line for 
"Fixes:" tag.

Thanks,
Tze Yee