Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
On Fri, Aug 12, 2022 at 07:03:26PM -0300, Daniel Henrique Barboza wrote:
> David,
>
> On 8/8/22 00:23, David Gibson wrote:
> > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
> > > At this moment, arm_load_dtb() can free machine->fdt when
> > > binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> > > retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> > > the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> > > machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> > > arm_load_dtb() will make machine->fdt point to an invalid memory region.
> > >
> > > This is not an issue right now because there's no code that access
> > > machine->fdt after arm_load_dtb(), but we're going to add a couple do
> > > FDT HMP commands that will rely on machine->fdt being valid.
> > >
> > > Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
> > > machine->fdt. This will allow the FDT of ARM machines that relies on
> > > arm_load_dtb() to be accessed later on.
> > >
> > > Since all ARM machines allocates the FDT only once, we don't need to
> > > worry about leaking the existing FDT during a machine reset (which is
> > > something that other machines have to look after, e.g. the ppc64 pSeries
> > > machine).
> > >
> > > Cc: Peter Maydell
> > > Cc: qemu-...@nongnu.org
> > > Signed-off-by: Daniel Henrique Barboza
> > > ---
> > > hw/arm/boot.c | 8 +++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index ada2717f76..9f5ceb62d2 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct
> > > arm_boot_info *binfo,
> > >*/
> > > rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
> > > -g_free(fdt);
> > > +/*
> > > + * Update the ms->fdt pointer to enable support for 'dumpdtb'
> > > + * and 'info fdt' commands. Use fdt_pack() to shrink the blob
> > > + * size we're going to store.
> > > + */
> > > +fdt_pack(fdt);
> > > +ms->fdt = fdt;
> > > return size;
> >
> > fdt_pack() could change (reduce) the effective size of the dtb blob,
> > so returning a 'size' value from above rather than the new value of
> > fdt_totalsize(fdt) doesn't see right.
>
> After some thought I think executing fdt_pack() like I'm doing here is not
> a good idea. The first problem is that I'm not returning the updated size,
> as you've said.
>
> But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the
> FDT **after** the machine store it in the guest physical memory. If I return
> the packed size, but the machine isn't packing the FDT before a
> cpu_physical_memory_write(), I'll be under-reporting the FDT size written.
Ah... good point.
> Machines such as e500 (patch 4) uses this returned value to put more stuff in
> the guest memory. In that case, returning a smaller size that what was
> actually
> written can cause the machine to overwrite the FDT by accident. In fact, only
> a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a
> fdt_pack() before a cpu_physical_memory_write(). So this change would be
> potentially harmful to a lot of people.
>
> One alternative would be to do a fdt_pack() before the machine writes the
> FDT in the guest memory, but that is too intrusive to do because I can't say
> if each of these machines will be OK with that. I have a hunch that it would
> be OK, but a hunch isn't going to cut it.
Hmm.. I'd be fairly confident that would be ok. It certainly should
be ok for the fdt content itself, fdt_pack() doesn't change that
semantically. If the machine is using that size to put stuff after, I
can't really see how that could break, either. Unless the machine
were using the fdtsize in one place and a fixed size somewhere else to
address the same data, which would be a bug.
> I'll just drop the fdt_pack() for each of these patches. If the machine code
> is already packing it, fine. If not, that's fine too. Each maintainer can
> assess whether packing the FDT is worth it or not.
That's probably reasonable for the time being.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
David,
On 8/8/22 00:23, David Gibson wrote:
On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
At this moment, arm_load_dtb() can free machine->fdt when
binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
machine->fdt. And, in that case, the existing g_free(fdt) at the end of
arm_load_dtb() will make machine->fdt point to an invalid memory region.
This is not an issue right now because there's no code that access
machine->fdt after arm_load_dtb(), but we're going to add a couple do
FDT HMP commands that will rely on machine->fdt being valid.
Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
machine->fdt. This will allow the FDT of ARM machines that relies on
arm_load_dtb() to be accessed later on.
Since all ARM machines allocates the FDT only once, we don't need to
worry about leaking the existing FDT during a machine reset (which is
something that other machines have to look after, e.g. the ppc64 pSeries
machine).
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
hw/arm/boot.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..9f5ceb62d2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
*binfo,
*/
rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
-g_free(fdt);
+/*
+ * Update the ms->fdt pointer to enable support for 'dumpdtb'
+ * and 'info fdt' commands. Use fdt_pack() to shrink the blob
+ * size we're going to store.
+ */
+fdt_pack(fdt);
+ms->fdt = fdt;
return size;
fdt_pack() could change (reduce) the effective size of the dtb blob,
so returning a 'size' value from above rather than the new value of
fdt_totalsize(fdt) doesn't see right.
After some thought I think executing fdt_pack() like I'm doing here is not
a good idea. The first problem is that I'm not returning the updated size,
as you've said.
But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the
FDT **after** the machine store it in the guest physical memory. If I return
the packed size, but the machine isn't packing the FDT before a
cpu_physical_memory_write(), I'll be under-reporting the FDT size written.
Machines such as e500 (patch 4) uses this returned value to put more stuff in
the guest memory. In that case, returning a smaller size that what was actually
written can cause the machine to overwrite the FDT by accident. In fact, only
a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a
fdt_pack() before a cpu_physical_memory_write(). So this change would be
potentially harmful to a lot of people.
One alternative would be to do a fdt_pack() before the machine writes the
FDT in the guest memory, but that is too intrusive to do because I can't say
if each of these machines will be OK with that. I have a hunch that it would
be OK, but a hunch isn't going to cut it.
I'll just drop the fdt_pack() for each of these patches. If the machine code
is already packing it, fine. If not, that's fine too. Each maintainer can
assess whether packing the FDT is worth it or not.
Thanks,
Daniel
I believe some of the other patches in the series have similar concerns.
Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
On 8/8/22 00:23, David Gibson wrote:
On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
At this moment, arm_load_dtb() can free machine->fdt when
binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
machine->fdt. And, in that case, the existing g_free(fdt) at the end of
arm_load_dtb() will make machine->fdt point to an invalid memory region.
This is not an issue right now because there's no code that access
machine->fdt after arm_load_dtb(), but we're going to add a couple do
FDT HMP commands that will rely on machine->fdt being valid.
Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
machine->fdt. This will allow the FDT of ARM machines that relies on
arm_load_dtb() to be accessed later on.
Since all ARM machines allocates the FDT only once, we don't need to
worry about leaking the existing FDT during a machine reset (which is
something that other machines have to look after, e.g. the ppc64 pSeries
machine).
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
hw/arm/boot.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..9f5ceb62d2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
*binfo,
*/
rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
-g_free(fdt);
+/*
+ * Update the ms->fdt pointer to enable support for 'dumpdtb'
+ * and 'info fdt' commands. Use fdt_pack() to shrink the blob
+ * size we're going to store.
+ */
+fdt_pack(fdt);
+ms->fdt = fdt;
return size;
fdt_pack() could change (reduce) the effective size of the dtb blob,
so returning a 'size' value from above rather than the new value of
fdt_totalsize(fdt) doesn't see right.
I believe some of the other patches in the series have similar concerns.
Ok! I'll revisit those patches and be sure to return the updated value
of the fdt.
Thanks,
Daniel
Re: [PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote:
> At this moment, arm_load_dtb() can free machine->fdt when
> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>
> This is not an issue right now because there's no code that access
> machine->fdt after arm_load_dtb(), but we're going to add a couple do
> FDT HMP commands that will rely on machine->fdt being valid.
>
> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
> machine->fdt. This will allow the FDT of ARM machines that relies on
> arm_load_dtb() to be accessed later on.
>
> Since all ARM machines allocates the FDT only once, we don't need to
> worry about leaking the existing FDT during a machine reset (which is
> something that other machines have to look after, e.g. the ppc64 pSeries
> machine).
>
> Cc: Peter Maydell
> Cc: qemu-...@nongnu.org
> Signed-off-by: Daniel Henrique Barboza
> ---
> hw/arm/boot.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ada2717f76..9f5ceb62d2 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
> *binfo,
> */
> rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
>
> -g_free(fdt);
> +/*
> + * Update the ms->fdt pointer to enable support for 'dumpdtb'
> + * and 'info fdt' commands. Use fdt_pack() to shrink the blob
> + * size we're going to store.
> + */
> +fdt_pack(fdt);
> +ms->fdt = fdt;
>
> return size;
fdt_pack() could change (reduce) the effective size of the dtb blob,
so returning a 'size' value from above rather than the new value of
fdt_totalsize(fdt) doesn't see right.
I believe some of the other patches in the series have similar concerns.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[PATCH for-7.2 v2 01/20] hw/arm: do not free machine->fdt in arm_load_dtb()
At this moment, arm_load_dtb() can free machine->fdt when
binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
machine->fdt. And, in that case, the existing g_free(fdt) at the end of
arm_load_dtb() will make machine->fdt point to an invalid memory region.
This is not an issue right now because there's no code that access
machine->fdt after arm_load_dtb(), but we're going to add a couple do
FDT HMP commands that will rely on machine->fdt being valid.
Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to
machine->fdt. This will allow the FDT of ARM machines that relies on
arm_load_dtb() to be accessed later on.
Since all ARM machines allocates the FDT only once, we don't need to
worry about leaking the existing FDT during a machine reset (which is
something that other machines have to look after, e.g. the ppc64 pSeries
machine).
Cc: Peter Maydell
Cc: qemu-...@nongnu.org
Signed-off-by: Daniel Henrique Barboza
---
hw/arm/boot.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ada2717f76..9f5ceb62d2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info
*binfo,
*/
rom_add_blob_fixed_as("dtb", fdt, size, addr, as);
-g_free(fdt);
+/*
+ * Update the ms->fdt pointer to enable support for 'dumpdtb'
+ * and 'info fdt' commands. Use fdt_pack() to shrink the blob
+ * size we're going to store.
+ */
+fdt_pack(fdt);
+ms->fdt = fdt;
return size;
--
2.36.1
