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

["Cédric Le Goater" <clg@kaod.org>]

On 7/30/26 15:53, Shrikanth Hegde wrote:
> 
> 
> 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.

Then do like XICS, which has BUG_ON(). I don't know why we took this
direction when XIVE was first introduced.

C. 
>>
>> 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/20260725202632.dcb325658896a470df91cf57@linux-foundation.org/ [1]
>> Fixes: 7dcc37b3eff9 ("powerpc/xive: Map one IPI interrupt per node")
>> Signed-off-by: Gou Hao <gouhao@uniontech.com>
>> Reviewed-by: Wentao Guan <guanwentao@uniontech.com>
>> Reviewed-by: jiazhenyuan <jiazhenyuan@uniontech.com>
>> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
>> Suggested-by: Cédric Le Goater <clg@kaod.org>
>> Suggested-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
>> Reviewed-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
>> 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;
>