Re: [PATCH v3 1/5] powerpc/xive: make xive IPI allocation NULL-safe
From: Shrikanth Hegde <hidden>
Date: 2026-07-30 13:54:20
Also in:
lkml
On 7/27/26 4:12 PM, Gou Hao wrote:
__GFP_NOFAIL should not be used in new code [1]. xive_init_ipis() allocates the xive_ipis array with __GFP_NOFAIL, which makes the subsequent NULL check unreachable dead code. Remove __GFP_NOFAIL so the allocation can fail, and make all xive_ipis access paths NULL-safe: - Return XIVE_BAD_IRQ from xive_ipi_cpu_to_irq() when xive_ipis is NULL. - Set xive_ipis to NULL after kfree() in the error path to prevent use-after-free. - Guard xive_setup_cpu_ipi() and xive_cleanup_cpu_ipi() against xive_ipi_irq == XIVE_BAD_IRQ to avoid dereferencing an uninitialized or already-freed xive_ipis array.
I would rather prefer a BUG_ON if the allocation fails. That keeps the earlier semantic. - If xive fails, then who will send the interrupts?. It is better to crash instead of leaving the system in weird state.
No functional change when allocation succeeds.
Please don't put a statement like this. It should be either no functional changes or function changes.
quoted hunk ↗ jump to hunk
Link: https://lore.kernel.org/all/20260725202632.dcb325658896a470df91cf57@linux-foundation.org/ (local) [1] Fixes: 7dcc37b3eff9 ("powerpc/xive: Map one IPI interrupt per node") Signed-off-by: Gou Hao <redacted> Reviewed-by: Wentao Guan <redacted> Reviewed-by: jiazhenyuan <redacted> Suggested-by: Andrew Morton <akpm@linux-foundation.org> Suggested-by: Cédric Le Goater <clg@kaod.org> Suggested-by: Mukesh Kumar Chaurasiya (IBM) <redacted> Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <redacted> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Andrew Morton <akpm@linux-foundation.org> --- arch/powerpc/sysdev/xive/common.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c index dadd1f46ec93..86c78af1f68e 100644 --- a/arch/powerpc/sysdev/xive/common.c +++ b/arch/powerpc/sysdev/xive/common.c@@ -74,6 +74,8 @@ static struct xive_ipi_desc { */ static unsigned int xive_ipi_cpu_to_irq(unsigned int cpu) { + if (!xive_ipis) + return XIVE_BAD_IRQ; return xive_ipis[early_cpu_to_node(cpu)].irq; } #endif@@ -1132,8 +1134,7 @@ static int __init xive_init_ipis(void) if (!ipi_domain) goto out_free_fwnode; - xive_ipis = kzalloc_objs(*xive_ipis, nr_node_ids, - GFP_KERNEL | __GFP_NOFAIL); + xive_ipis = kzalloc_objs(*xive_ipis, nr_node_ids, GFP_KERNEL); if (!xive_ipis) goto out_free_domain;@@ -1158,6 +1159,7 @@ static int __init xive_init_ipis(void) out_free_xive_ipis: kfree(xive_ipis); + xive_ipis = NULL; out_free_domain: irq_domain_remove(ipi_domain); out_free_fwnode:@@ -1190,6 +1192,9 @@ static int xive_setup_cpu_ipi(unsigned int cpu) pr_debug("Setting up IPI for CPU %d\n", cpu); + if (xive_ipi_irq == XIVE_BAD_IRQ) + return -EIO; + xc = per_cpu(xive_cpu, cpu); /* Check if we are already setup */@@ -1234,6 +1239,9 @@ noinstr static void xive_cleanup_cpu_ipi(unsigned int cpu, struct xive_cpu *xc) /* Disable the IPI and free the IRQ data */ + if (xive_ipi_irq == XIVE_BAD_IRQ) + return; + /* Already cleaned up ? */ if (xc->hw_ipi == XIVE_BAD_IRQ) return;