* QMP command dumpdtb crash bug
@ 2023-03-23 6:29 Markus Armbruster
2023-03-23 12:17 ` Daniel Henrique Barboza
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2023-03-23 6:29 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Daniel Henrique Barboza, Philippe Mathieu-Daudé, qemu-devel
Watch this:
$ gdb --args ../qemu/bld/qemu-system-aarch64 -S -M virt -display none -qmp stdio
[...]
(gdb) r
[...]
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 7}, "package": "v7.2.0-2331-gda89f78a7d"}, "capabilities": ["oob"]}}
[New Thread 0x7fffed62c6c0 (LWP 1021967)]
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
{"return": {}}
{"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
qmp_dumpdtb (filename=0x5555581c5170 "fdt.dtb", errp=errp@entry=0x7fffffffdae8)
at ../softmmu/device_tree.c:661
661 size = fdt_totalsize(current_machine->fdt);
current_machine->fdt is non-null here. The crash is within
fdt_totalsize().
I suspect ...
void qmp_dumpdtb(const char *filename, Error **errp)
{
g_autoptr(GError) err = NULL;
uint32_t size;
if (!current_machine->fdt) {
error_setg(errp, "This machine doesn't have a FDT");
return;
}
... we're missing an "FDT isn't ready" guard here.
size = fdt_totalsize(current_machine->fdt);
g_assert(size > 0);
if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
error_setg(errp, "Error saving FDT to file %s: %s",
filename, err->message);
}
}
Also, I think the error message "does not have a FDT" should say "an
FDT".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QMP command dumpdtb crash bug
2023-03-23 6:29 QMP command dumpdtb crash bug Markus Armbruster
@ 2023-03-23 12:17 ` Daniel Henrique Barboza
2023-03-23 13:29 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Henrique Barboza @ 2023-03-23 12:17 UTC (permalink / raw)
To: Markus Armbruster, Daniel Henrique Barboza
Cc: Philippe Mathieu-Daudé, qemu-devel
On 3/23/23 03:29, Markus Armbruster wrote:
> Watch this:
>
> $ gdb --args ../qemu/bld/qemu-system-aarch64 -S -M virt -display none -qmp stdio
> [...]
> (gdb) r
> [...]
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 7}, "package": "v7.2.0-2331-gda89f78a7d"}, "capabilities": ["oob"]}}
> [New Thread 0x7fffed62c6c0 (LWP 1021967)]
> {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
> {"return": {}}
> {"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
>
> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
> qmp_dumpdtb (filename=0x5555581c5170 "fdt.dtb", errp=errp@entry=0x7fffffffdae8)
> at ../softmmu/device_tree.c:661
> 661 size = fdt_totalsize(current_machine->fdt);
>
> current_machine->fdt is non-null here. The crash is within
> fdt_totalsize().
Back when I added this command [1] one of the patches of this series was:
[PATCH v8 03/16] hw/arm: do not free machine->fdt in arm_load_dtb()
https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04201.html
The patch aimed to address what you're seeing here. machine->fdt is not NULL,
but it was freed in arm_load_dtb() without assigning it to NULL and it contains
junk.
Back then this patch got no acks/reviews and got left behind. If I apply it now
at current master your example works.
>
> I suspect ...
>
> void qmp_dumpdtb(const char *filename, Error **errp)
> {
> g_autoptr(GError) err = NULL;
> uint32_t size;
>
> if (!current_machine->fdt) {
> error_setg(errp, "This machine doesn't have a FDT");
> return;
> }
>
> ... we're missing an "FDT isn't ready" guard here.
There might be a case where a guard would be needed, but for all ARM machines that
uses arm_load_dtb() I can say that the dumpdtb is broken regardless of whether you
attempt it during early boot or not.
One solution is to just apply the patch I mentioned above. Another solution is to
make a g_free(fdt) in arm_load_dtb and also do a ms->fdt = NULL to tell dumpdtb()
that there is no fdt available.
I don't see any particular reason why arm machines can't support this command, so
let me know and I can re-send that patch.
Thanks,
Daniel
[1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04190.html
>
> size = fdt_totalsize(current_machine->fdt);
>
> g_assert(size > 0);
>
> if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
> error_setg(errp, "Error saving FDT to file %s: %s",
> filename, err->message);
> }
> }
>
> Also, I think the error message "does not have a FDT" should say "an
> FDT".
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QMP command dumpdtb crash bug
2023-03-23 12:17 ` Daniel Henrique Barboza
@ 2023-03-23 13:29 ` Markus Armbruster
2023-03-23 13:38 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2023-03-23 13:29 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel Henrique Barboza, Daniel Henrique Barboza,
Philippe Mathieu-Daudé, qemu-devel
Peter, Daniel offers two ways to fix this bug (see below). Got a
preference?
Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:
> On 3/23/23 03:29, Markus Armbruster wrote:
>> Watch this:
>>
>> $ gdb --args ../qemu/bld/qemu-system-aarch64 -S -M virt -display none -qmp stdio
>> [...]
>> (gdb) r
>> [...]
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 7}, "package": "v7.2.0-2331-gda89f78a7d"}, "capabilities": ["oob"]}}
>> [New Thread 0x7fffed62c6c0 (LWP 1021967)]
>> {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
>> {"return": {}}
>> {"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
>>
>> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
>> qmp_dumpdtb (filename=0x5555581c5170 "fdt.dtb", errp=errp@entry=0x7fffffffdae8)
>> at ../softmmu/device_tree.c:661
>> 661 size = fdt_totalsize(current_machine->fdt);
>>
>> current_machine->fdt is non-null here. The crash is within
>> fdt_totalsize().
>
>
> Back when I added this command [1] one of the patches of this series was:
>
> [PATCH v8 03/16] hw/arm: do not free machine->fdt in arm_load_dtb()
>
> https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04201.html
>
> The patch aimed to address what you're seeing here. machine->fdt is not NULL,
> but it was freed in arm_load_dtb() without assigning it to NULL and it contains
> junk.
>
> Back then this patch got no acks/reviews and got left behind. If I apply it now
> at current master your example works.
>
>>
>> I suspect ...
>>
>> void qmp_dumpdtb(const char *filename, Error **errp)
>> {
>> g_autoptr(GError) err = NULL;
>> uint32_t size;
>>
>> if (!current_machine->fdt) {
>> error_setg(errp, "This machine doesn't have a FDT");
>> return;
>> }
>>
>> ... we're missing an "FDT isn't ready" guard here.
>
>
> There might be a case where a guard would be needed, but for all ARM machines that
> uses arm_load_dtb() I can say that the dumpdtb is broken regardless of whether you
> attempt it during early boot or not.
>
> One solution is to just apply the patch I mentioned above. Another solution is to
> make a g_free(fdt) in arm_load_dtb and also do a ms->fdt = NULL to tell dumpdtb()
> that there is no fdt available.
>
> I don't see any particular reason why arm machines can't support this command, so
> let me know and I can re-send that patch.
>
>
>
> Thanks,
>
>
> Daniel
>
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04190.html
>
>>
>> size = fdt_totalsize(current_machine->fdt);
>>
>> g_assert(size > 0);
>>
>> if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>> error_setg(errp, "Error saving FDT to file %s: %s",
>> filename, err->message);
>> }
>> }
>>
>> Also, I think the error message "does not have a FDT" should say "an
>> FDT".
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QMP command dumpdtb crash bug
2023-03-23 13:29 ` Markus Armbruster
@ 2023-03-23 13:38 ` Peter Maydell
2023-03-23 15:13 ` Daniel Henrique Barboza
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2023-03-23 13:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: Daniel Henrique Barboza, Daniel Henrique Barboza,
Philippe Mathieu-Daudé, qemu-devel
On Thu, 23 Mar 2023 at 13:29, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter, Daniel offers two ways to fix this bug (see below). Got a
> preference?
Not freeing seems the correct thing. As Daniel says, this
should have been a prerequisite for implementing the
command in the first place (you need to change the lifecycle
of the fdt blob from "delete when done with in the arm boot code"
to "delete on machine finalize"). It looks like somehow we added
the command but missed out on getting all of the prerequisite
patches in. (File under "need to be cautious about applying partial
patchsets", I guess.)
Did anything else from that initial patchset get omitted?
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QMP command dumpdtb crash bug
2023-03-23 13:38 ` Peter Maydell
@ 2023-03-23 15:13 ` Daniel Henrique Barboza
2023-03-23 18:41 ` Bernhard Beschow
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Henrique Barboza @ 2023-03-23 15:13 UTC (permalink / raw)
To: Peter Maydell, Markus Armbruster
Cc: Daniel Henrique Barboza, Philippe Mathieu-Daudé, qemu-devel
On 3/23/23 10:38, Peter Maydell wrote:
> On Thu, 23 Mar 2023 at 13:29, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter, Daniel offers two ways to fix this bug (see below). Got a
>> preference?
>
> Not freeing seems the correct thing. As Daniel says, this
> should have been a prerequisite for implementing the
> command in the first place (you need to change the lifecycle
> of the fdt blob from "delete when done with in the arm boot code"
> to "delete on machine finalize"). It looks like somehow we added
> the command but missed out on getting all of the prerequisite
> patches in. (File under "need to be cautious about applying partial
> patchsets", I guess.)
Yeah, I'm at fault here. I should've been more insistent about acking
the ARM patch. All other patches that we left behind was optional, meaning
that the machine wouldn't implement the command but nothing bad would happen,
but the ARM patch was kind of mandatory because arm_load_dtb() is
freeing ms->fdt without assigning it to NULL.
>
> Did anything else from that initial patchset get omitted?
Searching the ML I see that I sent a message saying that I pushed patches 1,
6 and 8-15 via ppc-next. This means that these patches got left behind:
2 hw/core: free ms->fdt in machine_finalize()
3 hw/arm: do not free machine->fdt in arm_load_dtb()
4 hw/mips: set machine->fdt in boston_mach_init()
5 hw/microblaze: set machine->fdt in microblaze_load_dtb()
7 hw/ppc: set machine->fdt in ppce500_load_device_tree()
15 hw/xtensa: set machine->fdt in xtfpga_init()
Patch 2 was suggested by Phil and changes the common code to free ms->fdt
during machine_finalize(). Can be re-sent I guess.
All other patches, aside from patch 3 from ARM, are optional because the
machine isn't freeing ms->fdt or anything like that.
I'll rebase and re-sent patch 3 as a bug fix. I'll re-sent the hw/core patch
as well for 8.1.
Daniel
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: QMP command dumpdtb crash bug
2023-03-23 15:13 ` Daniel Henrique Barboza
@ 2023-03-23 18:41 ` Bernhard Beschow
0 siblings, 0 replies; 6+ messages in thread
From: Bernhard Beschow @ 2023-03-23 18:41 UTC (permalink / raw)
To: qemu-devel, Daniel Henrique Barboza, Peter Maydell,
Markus Armbruster
Cc: Daniel Henrique Barboza, Philippe Mathieu-Daudé
Am 23. März 2023 15:13:28 UTC schrieb Daniel Henrique Barboza <dbarboza@ventanamicro.com>:
>
>
>On 3/23/23 10:38, Peter Maydell wrote:
>> On Thu, 23 Mar 2023 at 13:29, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Peter, Daniel offers two ways to fix this bug (see below). Got a
>>> preference?
>>
>> Not freeing seems the correct thing. As Daniel says, this
>> should have been a prerequisite for implementing the
>> command in the first place (you need to change the lifecycle
>> of the fdt blob from "delete when done with in the arm boot code"
>> to "delete on machine finalize"). It looks like somehow we added
>> the command but missed out on getting all of the prerequisite
>> patches in. (File under "need to be cautious about applying partial
>> patchsets", I guess.)
>
>Yeah, I'm at fault here. I should've been more insistent about acking
>the ARM patch. All other patches that we left behind was optional, meaning
>that the machine wouldn't implement the command but nothing bad would happen,
>but the ARM patch was kind of mandatory because arm_load_dtb() is
>freeing ms->fdt without assigning it to NULL.
>
>>
>> Did anything else from that initial patchset get omitted?
>
>Searching the ML I see that I sent a message saying that I pushed patches 1,
>6 and 8-15 via ppc-next. This means that these patches got left behind:
>
> 2 hw/core: free ms->fdt in machine_finalize()
> 3 hw/arm: do not free machine->fdt in arm_load_dtb()
> 4 hw/mips: set machine->fdt in boston_mach_init()
> 5 hw/microblaze: set machine->fdt in microblaze_load_dtb()
> 7 hw/ppc: set machine->fdt in ppce500_load_device_tree()
We dealt with e500 in a different series. So 7 is basically in 8.0 already (but in a different form).
Best regards,
Bernhard
>15 hw/xtensa: set machine->fdt in xtfpga_init()
>
>
>Patch 2 was suggested by Phil and changes the common code to free ms->fdt
>during machine_finalize(). Can be re-sent I guess.
>
>All other patches, aside from patch 3 from ARM, are optional because the
>machine isn't freeing ms->fdt or anything like that.
>
>
>I'll rebase and re-sent patch 3 as a bug fix. I'll re-sent the hw/core patch
>as well for 8.1.
>
>
>Daniel
>
>
>>
>> thanks
>> -- PMM
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-23 18:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-23 6:29 QMP command dumpdtb crash bug Markus Armbruster
2023-03-23 12:17 ` Daniel Henrique Barboza
2023-03-23 13:29 ` Markus Armbruster
2023-03-23 13:38 ` Peter Maydell
2023-03-23 15:13 ` Daniel Henrique Barboza
2023-03-23 18:41 ` Bernhard Beschow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).