* [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
@ 2026-09-30 18:09 Jyoti Gupta
2026-09-30 18:50 ` Amit Machhiwal
2026-10-03 9:32 ` Mukesh Kumar Chaurasiya
0 siblings, 2 replies; 6+ messages in thread
From: Jyoti Gupta @ 2026-09-30 18:09 UTC (permalink / raw)
To: linuxppc-dev
Cc: maddy, mpe, npiggin, chleroy, ritesh.list, sshegde, sourabhjain,
amachhiw, mkchauras
mpic_alloc() allocates struct mpic (and optionally the protected
sources bitmap) but does not free them when mpic_setup_error_int() or
irq_domain_create_linear() fails, leaking the memory.
Free the bitmap and the mpic in the common error path before dropping the
device node reference.
Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree node if NULL")
Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
---
arch/powerpc/sysdev/mpic.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 257886ec3f70..b21a880777cc 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
if (fsl_version >= 0x401) {
ret = mpic_setup_error_int(mpic, intvec_top - 13);
if (ret)
- return NULL;
+ goto err_of_node_put;
}
}
@@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct device_node *node,
intvec_top,
&mpic_host_ops, mpic);
- /*
- * FIXME: The code leaks the MPIC object and mappings here; this
- * is very unlikely to fail but it ought to be fixed anyways.
- */
if (mpic->irqhost == NULL)
- return NULL;
+ goto err_of_node_put;
/* Display version */
switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
@@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct device_node *node,
return mpic;
err_of_node_put:
+ if (mpic)
+ bitmap_free(mpic->protected);
+ kfree(mpic);
of_node_put(node);
return NULL;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
2026-09-30 18:09 [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths Jyoti Gupta
@ 2026-09-30 18:50 ` Amit Machhiwal
2026-10-01 11:59 ` Jyoti Gupta
2026-10-03 9:32 ` Mukesh Kumar Chaurasiya
1 sibling, 1 reply; 6+ messages in thread
From: Amit Machhiwal @ 2026-09-30 18:50 UTC (permalink / raw)
To: Jyoti Gupta
Cc: linuxppc-dev, maddy, mpe, npiggin, chleroy, ritesh.list, sshegde,
sourabhjain, amachhiw, mkchauras
On 2026/09/30 11:39 PM, Jyoti Gupta wrote:
> mpic_alloc() allocates struct mpic (and optionally the protected
> sources bitmap) but does not free them when mpic_setup_error_int() or
> irq_domain_create_linear() fails, leaking the memory.
>
> Free the bitmap and the mpic in the common error path before dropping the
> device node reference.
>
> Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree node if NULL")
> Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
> ---
> arch/powerpc/sysdev/mpic.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
Thanks for addressing the comments from v1.
A couple of minor suggestions:
1. Changelog & Links: It is always a good practice to include a changelog below
the "---" line detailing the changes made in this version (e.g. "v1 -> v2:
...") along with a link to the previous version(s) on lore.kernel.org. This
helps reviewers and maintainers quickly track the evolution of the patch.
2. Minor nit in cleanup logic below:
>
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 257886ec3f70..b21a880777cc 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> if (fsl_version >= 0x401) {
> ret = mpic_setup_error_int(mpic, intvec_top - 13);
> if (ret)
> - return NULL;
> + goto err_of_node_put;
> }
>
> }
> @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> intvec_top,
> &mpic_host_ops, mpic);
>
> - /*
> - * FIXME: The code leaks the MPIC object and mappings here; this
> - * is very unlikely to fail but it ought to be fixed anyways.
> - */
> if (mpic->irqhost == NULL)
> - return NULL;
> + goto err_of_node_put;
>
> /* Display version */
> switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
> @@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> return mpic;
>
> err_of_node_put:
> + if (mpic)
> + bitmap_free(mpic->protected);
> + kfree(mpic);
Both bitmap_free() and kfree() safely handle NULL pointers (and mpic is
guaranteed to be non-NULL at this label anyway), so the 'if (mpic)' check is
redundant.
Other than that, the patch looks good to me. So, with this fixed, please feel
free to add:
Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>
Thanks,
Amit
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
2026-09-30 18:50 ` Amit Machhiwal
@ 2026-10-01 11:59 ` Jyoti Gupta
2026-10-05 7:44 ` Amit Machhiwal
0 siblings, 1 reply; 6+ messages in thread
From: Jyoti Gupta @ 2026-10-01 11:59 UTC (permalink / raw)
To: amachhiw
Cc: linuxppc-dev, maddy, mpe, npiggin, chleroy, ritesh.list, sshegde,
sourabhjain, mkchauras
On 2026-10-01 00:20, Amit Machhiwal wrote:
> On 2026/09/30 11:39 PM, Jyoti Gupta wrote:
>> mpic_alloc() allocates struct mpic (and optionally the protected
>> sources bitmap) but does not free them when mpic_setup_error_int() or
>> irq_domain_create_linear() fails, leaking the memory.
>>
>> Free the bitmap and the mpic in the common error path before dropping
>> the
>> device node reference.
>>
>> Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree
>> node if NULL")
>> Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
>> ---
>> arch/powerpc/sysdev/mpic.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> Thanks for addressing the comments from v1.
>
> A couple of minor suggestions:
>
> 1. Changelog & Links: It is always a good practice to include a
> changelog below
> the "---" line detailing the changes made in this version (e.g. "v1
> -> v2:
> ...") along with a link to the previous version(s) on
> lore.kernel.org. This
> helps reviewers and maintainers quickly track the evolution of the
> patch.
>
Got it, thank you for the suggestion. I'll keep it in mind.
> 2. Minor nit in cleanup logic below:
>
>>
>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
>> index 257886ec3f70..b21a880777cc 100644
>> --- a/arch/powerpc/sysdev/mpic.c
>> +++ b/arch/powerpc/sysdev/mpic.c
>> @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct
>> device_node *node,
>> if (fsl_version >= 0x401) {
>> ret = mpic_setup_error_int(mpic, intvec_top - 13);
>> if (ret)
>> - return NULL;
>> + goto err_of_node_put;
>> }
>>
>> }
>> @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct
>> device_node *node,
>> intvec_top,
>> &mpic_host_ops, mpic);
>>
>> - /*
>> - * FIXME: The code leaks the MPIC object and mappings here; this
>> - * is very unlikely to fail but it ought to be fixed anyways.
>> - */
>> if (mpic->irqhost == NULL)
>> - return NULL;
>> + goto err_of_node_put;
>>
>> /* Display version */
>> switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
>> @@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct
>> device_node *node,
>> return mpic;
>>
>> err_of_node_put:
>> + if (mpic)
>> + bitmap_free(mpic->protected);
>> + kfree(mpic);
>
> Both bitmap_free() and kfree() safely handle NULL pointers (and mpic is
> guaranteed to be non-NULL at this label anyway), so the 'if (mpic)'
> check is
> redundant.
>
Thanks for the review.
Regarding the 'if (mpic)' check, I think we may need to keep it. While
bitmap_free() and kfree() both safely handle NULL, the concern here is
dereferencing mpic to obtain mpic->protected. There are two paths that
reach err_of_node_put before mpic is allocated (line 1276):
of_address_to_resource() failure at line 1258 and kzalloc_obj()
returning
NULL at line 1278. In both cases, mpic->protected would dereference a
NULL/garbage pointer and crash.
Please let me know if I am missing something!
Thanks,
Jyoti
> Other than that, the patch looks good to me. So, with this fixed,
> please feel
> free to add:
>
> Reviewed-by: Amit Machhiwal <amachhiw@linux.ibm.com>
>
> Thanks,
> Amit
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
2026-09-30 18:09 [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths Jyoti Gupta
2026-09-30 18:50 ` Amit Machhiwal
@ 2026-10-03 9:32 ` Mukesh Kumar Chaurasiya
1 sibling, 0 replies; 6+ messages in thread
From: Mukesh Kumar Chaurasiya @ 2026-10-03 9:32 UTC (permalink / raw)
To: Jyoti Gupta
Cc: linuxppc-dev, maddy, mpe, npiggin, chleroy, ritesh.list, sshegde,
sourabhjain, amachhiw
On Wed, Sep 30, 2026 at 11:39:19PM +0530, Jyoti Gupta wrote:
> mpic_alloc() allocates struct mpic (and optionally the protected
> sources bitmap) but does not free them when mpic_setup_error_int() or
> irq_domain_create_linear() fails, leaking the memory.
>
> Free the bitmap and the mpic in the common error path before dropping the
> device node reference.
>
> Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree node if NULL")
> Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
> ---
> arch/powerpc/sysdev/mpic.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 257886ec3f70..b21a880777cc 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> if (fsl_version >= 0x401) {
> ret = mpic_setup_error_int(mpic, intvec_top - 13);
> if (ret)
> - return NULL;
> + goto err_of_node_put;
> }
>
> }
> @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> intvec_top,
> &mpic_host_ops, mpic);
>
> - /*
> - * FIXME: The code leaks the MPIC object and mappings here; this
> - * is very unlikely to fail but it ought to be fixed anyways.
> - */
> if (mpic->irqhost == NULL)
> - return NULL;
> + goto err_of_node_put;
>
> /* Display version */
> switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
> @@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct device_node *node,
> return mpic;
>
> err_of_node_put:
> + if (mpic)
> + bitmap_free(mpic->protected);
> + kfree(mpic);
> of_node_put(node);
> return NULL;
> }
> --
> 2.55.0
>
>
LGTM
Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
2026-10-01 11:59 ` Jyoti Gupta
@ 2026-10-05 7:44 ` Amit Machhiwal
2026-10-05 15:18 ` Jyoti Gupta
0 siblings, 1 reply; 6+ messages in thread
From: Amit Machhiwal @ 2026-10-05 7:44 UTC (permalink / raw)
To: Jyoti Gupta
Cc: amachhiw, linuxppc-dev, maddy, mpe, npiggin, chleroy, ritesh.list,
sshegde, sourabhjain, mkchauras
Hi Jyoti,
Please find my responses inline below.
On 2026/10/01 05:29 PM, Jyoti Gupta wrote:
> On 2026-10-01 00:20, Amit Machhiwal wrote:
> > On 2026/09/30 11:39 PM, Jyoti Gupta wrote:
> > > mpic_alloc() allocates struct mpic (and optionally the protected
> > > sources bitmap) but does not free them when mpic_setup_error_int() or
> > > irq_domain_create_linear() fails, leaking the memory.
> > >
> > > Free the bitmap and the mpic in the common error path before
> > > dropping the
> > > device node reference.
> > >
> > > Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree
> > > node if NULL")
> > > Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
> > > ---
> > > arch/powerpc/sysdev/mpic.c | 11 +++++------
> > > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > Thanks for addressing the comments from v1.
> >
> > A couple of minor suggestions:
> >
> > 1. Changelog & Links: It is always a good practice to include a
> > changelog below
> > the "---" line detailing the changes made in this version (e.g. "v1
> > -> v2:
> > ...") along with a link to the previous version(s) on
> > lore.kernel.org. This
> > helps reviewers and maintainers quickly track the evolution of the
> > patch.
> >
>
> Got it, thank you for the suggestion. I'll keep it in mind.
>
> > 2. Minor nit in cleanup logic below:
> >
> > >
> > > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> > > index 257886ec3f70..b21a880777cc 100644
> > > --- a/arch/powerpc/sysdev/mpic.c
> > > +++ b/arch/powerpc/sysdev/mpic.c
> > > @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct
> > > device_node *node,
> > > if (fsl_version >= 0x401) {
> > > ret = mpic_setup_error_int(mpic, intvec_top - 13);
> > > if (ret)
> > > - return NULL;
> > > + goto err_of_node_put;
> > > }
> > >
> > > }
> > > @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct
> > > device_node *node,
> > > intvec_top,
> > > &mpic_host_ops, mpic);
> > >
> > > - /*
> > > - * FIXME: The code leaks the MPIC object and mappings here; this
> > > - * is very unlikely to fail but it ought to be fixed anyways.
> > > - */
> > > if (mpic->irqhost == NULL)
> > > - return NULL;
> > > + goto err_of_node_put;
> > >
> > > /* Display version */
> > > switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
> > > @@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct
> > > device_node *node,
> > > return mpic;
> > >
> > > err_of_node_put:
> > > + if (mpic)
> > > + bitmap_free(mpic->protected);
> > > + kfree(mpic);
> >
> > Both bitmap_free() and kfree() safely handle NULL pointers (and mpic is
> > guaranteed to be non-NULL at this label anyway), so the 'if (mpic)'
> > check is
> > redundant.
> >
>
> Thanks for the review.
>
> Regarding the 'if (mpic)' check, I think we may need to keep it. While
> bitmap_free() and kfree() both safely handle NULL, the concern here is
> dereferencing mpic to obtain mpic->protected. There are two paths that
> reach err_of_node_put before mpic is allocated (line 1276):
> of_address_to_resource() failure at line 1258 and kzalloc_obj() returning
> NULL at line 1278. In both cases, mpic->protected would dereference a
> NULL/garbage pointer and crash.
Good catch on the early error paths before mpic allocation!
However, in v2, 'struct mpic *mpic' is left uninitialized at function entry
(since the '= NULL' initialization was dropped).
Looking more closely, if of_address_to_resource() fails at line 1258, control
jumps to err_of_node_put where 'mpic' contains uninitialized stack garbage. In
that case, 'if (mpic)' evaluates to true, leading to an invalid pointer
dereference when evaluating mpic->protected and a crash on kfree(mpic).
Rather than initializing mpic to NULL and checking 'if (mpic)' on error paths,
the preferred approach in Linux is to use layered error labels. In fact,
Documentation/process/coding-style.rst (Section 7, "Centralized exiting of
functions") specifically describes this pattern when discussing "one err bugs":
A common type of bug to be aware of is ``one err bugs`` which look like this:
.. code-block:: c
err:
kfree(foo->bar);
kfree(foo);
return ret;
The bug in this code is that on some exit paths ``foo`` is NULL. Normally the
fix for this is to split it up into two error labels ``err_free_bar:`` and
``err_free_foo:``:
.. code-block:: c
err_free_bar:
kfree(foo->bar);
err_free_foo:
kfree(foo);
return ret;
Applying that pattern here gives:
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 257886ec3f70..d799cf4da7bd 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct device_node *node,
if (fsl_version >= 0x401) {
ret = mpic_setup_error_int(mpic, intvec_top - 13);
if (ret)
- return NULL;
+ goto err_free_mpic;
}
}
@@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct device_node *node,
intvec_top,
&mpic_host_ops, mpic);
- /*
- * FIXME: The code leaks the MPIC object and mappings here; this
- * is very unlikely to fail but it ought to be fixed anyways.
- */
if (mpic->irqhost == NULL)
- return NULL;
+ goto err_free_mpic;
/* Display version */
switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
@@ -1526,6 +1522,9 @@ struct mpic * __init mpic_alloc(struct device_node *node,
return mpic;
+err_free_mpic:
+ bitmap_free(mpic->protected);
+ kfree(mpic);
err_of_node_put:
of_node_put(node);
return NULL;
This ensures:
1. Early error paths (of_address_to_resource and kzalloc_obj failures) jump
directly to err_of_node_put without touching mpic.
2. Later error paths jump to err_free_mpic where mpic is guaranteed to be valid.
3. No 'if (mpic)' check or 'struct mpic *mpic = NULL;' is needed.
What do you think?
Thanks,
Amit
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths
2026-10-05 7:44 ` Amit Machhiwal
@ 2026-10-05 15:18 ` Jyoti Gupta
0 siblings, 0 replies; 6+ messages in thread
From: Jyoti Gupta @ 2026-10-05 15:18 UTC (permalink / raw)
To: amachhiw
Cc: linuxppc-dev, maddy, mpe, npiggin, chleroy, ritesh.list, sshegde,
sourabhjain, mkchauras
Hi Amit ,
Please refer to my inline comments below.
On 2026-10-05 13:14, Amit Machhiwal wrote:
> Hi Jyoti,
>
> Please find my responses inline below.
>
> On 2026/10/01 05:29 PM, Jyoti Gupta wrote:
>> On 2026-10-01 00:20, Amit Machhiwal wrote:
>> > On 2026/09/30 11:39 PM, Jyoti Gupta wrote:
>> > > mpic_alloc() allocates struct mpic (and optionally the protected
>> > > sources bitmap) but does not free them when mpic_setup_error_int() or
>> > > irq_domain_create_linear() fails, leaking the memory.
>> > >
>> > > Free the bitmap and the mpic in the common error path before
>> > > dropping the
>> > > device node reference.
>> > >
>> > > Fixes: 996983b75ceb ("powerpc/mpic: Search for open-pic device-tree
>> > > node if NULL")
>> > > Signed-off-by: Jyoti Gupta <jyoti@linux.ibm.com>
>> > > ---
>> > > arch/powerpc/sysdev/mpic.c | 11 +++++------
>> > > 1 file changed, 5 insertions(+), 6 deletions(-)
>> >
>> > Thanks for addressing the comments from v1.
>> >
>> > A couple of minor suggestions:
>> >
>> > 1. Changelog & Links: It is always a good practice to include a
>> > changelog below
>> > the "---" line detailing the changes made in this version (e.g. "v1
>> > -> v2:
>> > ...") along with a link to the previous version(s) on
>> > lore.kernel.org. This
>> > helps reviewers and maintainers quickly track the evolution of the
>> > patch.
>> >
>>
>> Got it, thank you for the suggestion. I'll keep it in mind.
>>
>> > 2. Minor nit in cleanup logic below:
>> >
>> > >
>> > > diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
>> > > index 257886ec3f70..b21a880777cc 100644
>> > > --- a/arch/powerpc/sysdev/mpic.c
>> > > +++ b/arch/powerpc/sysdev/mpic.c
>> > > @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct
>> > > device_node *node,
>> > > if (fsl_version >= 0x401) {
>> > > ret = mpic_setup_error_int(mpic, intvec_top - 13);
>> > > if (ret)
>> > > - return NULL;
>> > > + goto err_of_node_put;
>> > > }
>> > >
>> > > }
>> > > @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct
>> > > device_node *node,
>> > > intvec_top,
>> > > &mpic_host_ops, mpic);
>> > >
>> > > - /*
>> > > - * FIXME: The code leaks the MPIC object and mappings here; this
>> > > - * is very unlikely to fail but it ought to be fixed anyways.
>> > > - */
>> > > if (mpic->irqhost == NULL)
>> > > - return NULL;
>> > > + goto err_of_node_put;
>> > >
>> > > /* Display version */
>> > > switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
>> > > @@ -1527,6 +1523,9 @@ struct mpic * __init mpic_alloc(struct
>> > > device_node *node,
>> > > return mpic;
>> > >
>> > > err_of_node_put:
>> > > + if (mpic)
>> > > + bitmap_free(mpic->protected);
>> > > + kfree(mpic);
>> >
>> > Both bitmap_free() and kfree() safely handle NULL pointers (and mpic is
>> > guaranteed to be non-NULL at this label anyway), so the 'if (mpic)'
>> > check is
>> > redundant.
>> >
>>
>> Thanks for the review.
>>
>> Regarding the 'if (mpic)' check, I think we may need to keep it. While
>> bitmap_free() and kfree() both safely handle NULL, the concern here is
>> dereferencing mpic to obtain mpic->protected. There are two paths that
>> reach err_of_node_put before mpic is allocated (line 1276):
>> of_address_to_resource() failure at line 1258 and kzalloc_obj()
>> returning
>> NULL at line 1278. In both cases, mpic->protected would dereference a
>> NULL/garbage pointer and crash.
>
> Good catch on the early error paths before mpic allocation!
>
> However, in v2, 'struct mpic *mpic' is left uninitialized at function
> entry
> (since the '= NULL' initialization was dropped).
>
> Looking more closely, if of_address_to_resource() fails at line 1258,
> control
> jumps to err_of_node_put where 'mpic' contains uninitialized stack
> garbage. In
> that case, 'if (mpic)' evaluates to true, leading to an invalid pointer
> dereference when evaluating mpic->protected and a crash on kfree(mpic).
>
> Rather than initializing mpic to NULL and checking 'if (mpic)' on error
> paths,
> the preferred approach in Linux is to use layered error labels. In
> fact,
> Documentation/process/coding-style.rst (Section 7, "Centralized exiting
> of
> functions") specifically describes this pattern when discussing "one
> err bugs":
>
>
> A common type of bug to be aware of is ``one err bugs`` which look
> like this:
>
> .. code-block:: c
>
> err:
> kfree(foo->bar);
> kfree(foo);
> return ret;
>
> The bug in this code is that on some exit paths ``foo`` is NULL.
> Normally the
> fix for this is to split it up into two error labels
> ``err_free_bar:`` and
> ``err_free_foo:``:
>
> .. code-block:: c
>
> err_free_bar:
> kfree(foo->bar);
> err_free_foo:
> kfree(foo);
> return ret;
>
> Applying that pattern here gives:
>
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 257886ec3f70..d799cf4da7bd 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -1389,7 +1389,7 @@ struct mpic * __init mpic_alloc(struct
> device_node *node,
> if (fsl_version >= 0x401) {
> ret = mpic_setup_error_int(mpic, intvec_top -
> 13);
> if (ret)
> - return NULL;
> + goto err_free_mpic;
> }
>
> }
> @@ -1488,12 +1488,8 @@ struct mpic * __init mpic_alloc(struct
> device_node *node,
> intvec_top,
> &mpic_host_ops, mpic);
>
> - /*
> - * FIXME: The code leaks the MPIC object and mappings here;
> this
> - * is very unlikely to fail but it ought to be fixed anyways.
> - */
> if (mpic->irqhost == NULL)
> - return NULL;
> + goto err_free_mpic;
>
> /* Display version */
> switch (greg_feature & MPIC_GREG_FEATURE_VERSION_MASK) {
> @@ -1526,6 +1522,9 @@ struct mpic * __init mpic_alloc(struct
> device_node *node,
>
> return mpic;
>
> +err_free_mpic:
> + bitmap_free(mpic->protected);
> + kfree(mpic);
> err_of_node_put:
> of_node_put(node);
> return NULL;
>
> This ensures:
>
> 1. Early error paths (of_address_to_resource and kzalloc_obj failures)
> jump
> directly to err_of_node_put without touching mpic.
> 2. Later error paths jump to err_free_mpic where mpic is guaranteed to
> be valid.
> 3. No 'if (mpic)' check or 'struct mpic *mpic = NULL;' is needed.
>
Thank you for the detailed explanation.
> What do you think?
>
Yes, this sounds more accurate — the layered error labels are clearly
the right approach here. Will fix it in v3.
Thanks,
Jyoti
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-05 15:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-30 18:09 [PATCH v2] powerpc/mpic: Fix memory leak of mpic on error paths Jyoti Gupta
2026-09-30 18:50 ` Amit Machhiwal
2026-10-01 11:59 ` Jyoti Gupta
2026-10-05 7:44 ` Amit Machhiwal
2026-10-05 15:18 ` Jyoti Gupta
2026-10-03 9:32 ` Mukesh Kumar Chaurasiya
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.