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 <[email protected]>
> > ---
> > 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.
