Re: [PATCH v3 1/5] powerpc/xive: make xive IPI allocation NULL-safe

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;