| Message ID | 20250319020847.1511759-5-maobibo@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/loongarch: Solve some issues reported from coccinelle | expand |
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? If failure is impossible, the code before the patch is harmlessly wrong. 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> > --- > hw/loongarch/virt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c > index a9fab39dd8..f10a4704ab 100644 > --- a/hw/loongarch/virt.c > +++ b/hw/loongarch/virt.c > @@ -327,7 +327,6 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) > MachineClass *mc = MACHINE_GET_CLASS(ms); > const CPUArchIdList *possible_cpus; > CPUState *cs; > - Error *err = NULL; > > /* cpu nodes */ > possible_cpus = mc->possible_cpu_arch_ids(ms); > @@ -337,8 +336,10 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) > continue; > } > > - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), &err); > - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), &err); > + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), > + &error_abort); > + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), > + &error_abort); > } > }
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. 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> >> --- >> hw/loongarch/virt.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c >> index a9fab39dd8..f10a4704ab 100644 >> --- a/hw/loongarch/virt.c >> +++ b/hw/loongarch/virt.c >> @@ -327,7 +327,6 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) >> MachineClass *mc = MACHINE_GET_CLASS(ms); >> const CPUArchIdList *possible_cpus; >> CPUState *cs; >> - Error *err = NULL; >> >> /* cpu nodes */ >> possible_cpus = mc->possible_cpu_arch_ids(ms); >> @@ -337,8 +336,10 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) >> continue; >> } >> >> - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), &err); >> - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), &err); >> + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), >> + &error_abort); >> + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), >> + &error_abort); >> } >> } >
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>
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index a9fab39dd8..f10a4704ab 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -327,7 +327,6 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) MachineClass *mc = MACHINE_GET_CLASS(ms); const CPUArchIdList *possible_cpus; CPUState *cs; - Error *err = NULL; /* cpu nodes */ possible_cpus = mc->possible_cpu_arch_ids(ms); @@ -337,8 +336,10 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState *lvms) continue; } - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), &err); - hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), &err); + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), + &error_abort); + hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), + &error_abort); } }
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. Here set dest error object with error_abort, rather than local variable, so application will abort to run if there is error. 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> --- hw/loongarch/virt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)