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