Re: [PATCH v4 4/4] target/loongarch: Set dest error with error_abort in virt_cpu_irq_init

[Markus Armbruster <armbru@redhat.com>]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/19 下午2:09, Markus Armbruster wrote:
>> Bibo Mao <maobibo@loongson.cn> writes:
>> 
>>> In function virt_cpu_irq_init(), there is notification with ipi and extioi
>>> interrupt controller for cpu creation. Local variable with error type is
>>> used, however there is no check with its return value.
>> 
>> Good catch.
>> 
>> When the first call fails, we pass non-null @err to the second call,
>> which is wrong.  If that one also fails, it'll likely trip
>> error_setv()'s assertion.
>> 
>>> Here set dest error object with error_abort, rather than local variable, so
>>> application will abort to run if there is error.
>> 
>> Why is failure impossible there?
> In plug hanlder of extioi/ipi, there is only warn_report() if object is 
> not TYPE_LOONGARCH_CPU, parameter errp is not changed.
>
> With caller funciton virt_cpu_irq_init(), DEVICE(cs) is object with type 
> TYPE_LOONGARCH_CPU always, so failure is impossible here.
>
>> 
>> If failure is impossible, the code before the patch is harmlessly wrong.
> yes, it is harmlessly wrong.

Could use something like

    target/loongarch: Clean up virt_cpu_irq_init() error handling

    The Error ** argument must be NULL, &error_abort, &error_fatal, or a
    pointer to a variable containing NULL.  Passing an argument of the
    latter kind twice without clearing it in between is wrong: if the
    first call sets an error, it no longer points to NULL for the second
    call.
    
    virt_cpu_irq_init() is wrong that way: it passes &err to
    hotplug_handler_plug() twice.  If both calls failed, this could trip
    error_setv()'s assertion.  Moreover, if just one fails, the Error
    object leaks.  Fortunately, these calls can't actually fail.

    Messed up in commit 50ebc3fc47f7 (hw/intc/loongarch_ipi: Notify ipi
    object when cpu is plugged) and commit 087a23a87c57
    (hw/intc/loongarch_extioi: Use cpu plug notification).

    Clean this up by clearing passing &error_abort instead.

    Signed-off-by: Bibo Mao <maobibo@loongson.cn>

Note: I replaced the Fixes: tags by a "Messed up ..." paragraph, because
there is no bug to fix according to your explanation.

With something like that:
Acked-by: Markus Armbruster <armbru@redhat.com>

> Regards
> Bibo Mao
>> 
>> If failure is possible, the code before the patch has a crash bug, and
>> the patch makes it crash harder, i.e. when either call fails instead of
>> when both fail.
>> 
>>> Fixes: 50ebc3fc47fe (hw/intc/loongarch_ipi: Notify ipi object when cpu is plugged)
>>> Fixes: 087a23a87c57 (hw/intc/loongarch_extioi: Use cpu plug notification)
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>