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.
Link:
https://lore.kernel.org/all/[email protected]/
[1]
Fixes: 7dcc37b3eff9 ("powerpc/xive: Map one IPI interrupt per node")
Signed-off-by: Gou Hao <[email protected]>
Reviewed-by: Wentao Guan <[email protected]>
Reviewed-by: jiazhenyuan <[email protected]>
Suggested-by: Andrew Morton <[email protected]>
Suggested-by: Cédric Le Goater <[email protected]>
Suggested-by: Mukesh Kumar Chaurasiya (IBM) <[email protected]>
Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <[email protected]>
Reviewed-by: Cédric Le Goater <[email protected]>
Reviewed-by: Andrew Morton <[email protected]>
---
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;
