| Message ID | 20211221155230.1532850-1-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3] arm64: Unhash early pointer print plus improve comment | expand |
On 2021-12-21 15:52, Guilherme G. Piccoli wrote: > When facing a really early issue on DT parsing we have currently > a message that shows both the physical and virtual address of the FDT. > The printk pointer modifier for the virtual address shows a hashed > address there unless the user provides "no_hash_pointers" parameter in > the command-line. The situation in which this message shows-up is a bit > more serious though: the boot process is broken, nothing can be done > (even an oops is too much for this early stage) so we have this message > as a last resort in order to help debug bootloader issues, for example. > Hence, we hereby change that to "%px" in order to make debugging easy, > there's not much information leak risk in such early boot failure. > > Also, we tried to improve a bit the commenting on that function, given > that if kernel fails there, it just hangs forever in a cpu_relax() loop. > The reason we cannot BUG/panic is that is too early to do so; thanks to > Mark Brown for pointing that on IRC and thanks Robin Murphy for the good > pointer hash discussion in the mailing-list. Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > > V3: > * Improved commit message (thanks Robin!); > * Fixed comment style. > > arch/arm64/kernel/setup.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index be5f85b0a24d..a80430550a73 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -189,11 +189,16 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) > > if (!dt_virt || !early_init_dt_scan(dt_virt)) { > pr_crit("\n" > - "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n" > + "Error: invalid device tree blob at physical address %pa (virtual address 0x%px)\n" > "The dtb must be 8-byte aligned and must not exceed 2 MB in size\n" > "\nPlease check your bootloader.", > &dt_phys, dt_virt); > > + /* > + * Note that in this _really_ early stage we cannot even BUG() > + * or oops, so the least terrible thing to do is cpu_relax(), > + * or else we could end-up printing non-initialized data, etc. > + */ > while (true) > cpu_relax(); > }
Hello, On Tue, Dec 21, 2021 at 12:52:30PM -0300, Guilherme G. Piccoli wrote: > When facing a really early issue on DT parsing we have currently > a message that shows both the physical and virtual address of the FDT. > The printk pointer modifier for the virtual address shows a hashed > address there unless the user provides "no_hash_pointers" parameter in > the command-line. The situation in which this message shows-up is a bit > more serious though: the boot process is broken, nothing can be done > (even an oops is too much for this early stage) so we have this message > as a last resort in order to help debug bootloader issues, for example. > Hence, we hereby change that to "%px" in order to make debugging easy, > there's not much information leak risk in such early boot failure. > > Also, we tried to improve a bit the commenting on that function, given > that if kernel fails there, it just hangs forever in a cpu_relax() loop. > The reason we cannot BUG/panic is that is too early to do so; thanks to > Mark Brown for pointing that on IRC and thanks Robin Murphy for the good > pointer hash discussion in the mailing-list. > > Cc: Mark Brown <broonie@kernel.org> > Cc: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> not sure why I was Cc:d for this patch, but it looks good to me. If I had done it, I would have added a comment about the px, but I like it as is, too. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
On Tue, 21 Dec 2021 12:52:30 -0300, Guilherme G. Piccoli wrote: > When facing a really early issue on DT parsing we have currently > a message that shows both the physical and virtual address of the FDT. > The printk pointer modifier for the virtual address shows a hashed > address there unless the user provides "no_hash_pointers" parameter in > the command-line. The situation in which this message shows-up is a bit > more serious though: the boot process is broken, nothing can be done > (even an oops is too much for this early stage) so we have this message > as a last resort in order to help debug bootloader issues, for example. > Hence, we hereby change that to "%px" in order to make debugging easy, > there's not much information leak risk in such early boot failure. > > [...] Applied to arm64 (for-next/misc), thanks! [1/1] arm64: Unhash early pointer print plus improve comment https://git.kernel.org/arm64/c/31e833b20312
On 22/12/2021 03:51, Uwe Kleine-König wrote: > Hello, > >[...] > not sure why I was Cc:d for this patch, but it looks good to me. If I > had done it, I would have added a comment about the px, but I like it as > is, too. > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Best regards > Uwe > Thanks Uwe! You're CCed because you suggested me to do this patch on IRC, when I talked to Mark heheh - so I thought in adding you to the loop =)
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index be5f85b0a24d..a80430550a73 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -189,11 +189,16 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) if (!dt_virt || !early_init_dt_scan(dt_virt)) { pr_crit("\n" - "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n" + "Error: invalid device tree blob at physical address %pa (virtual address 0x%px)\n" "The dtb must be 8-byte aligned and must not exceed 2 MB in size\n" "\nPlease check your bootloader.", &dt_phys, dt_virt); + /* + * Note that in this _really_ early stage we cannot even BUG() + * or oops, so the least terrible thing to do is cpu_relax(), + * or else we could end-up printing non-initialized data, etc. + */ while (true) cpu_relax(); }
When facing a really early issue on DT parsing we have currently a message that shows both the physical and virtual address of the FDT. The printk pointer modifier for the virtual address shows a hashed address there unless the user provides "no_hash_pointers" parameter in the command-line. The situation in which this message shows-up is a bit more serious though: the boot process is broken, nothing can be done (even an oops is too much for this early stage) so we have this message as a last resort in order to help debug bootloader issues, for example. Hence, we hereby change that to "%px" in order to make debugging easy, there's not much information leak risk in such early boot failure. Also, we tried to improve a bit the commenting on that function, given that if kernel fails there, it just hangs forever in a cpu_relax() loop. The reason we cannot BUG/panic is that is too early to do so; thanks to Mark Brown for pointing that on IRC and thanks Robin Murphy for the good pointer hash discussion in the mailing-list. Cc: Mark Brown <broonie@kernel.org> Cc: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- V3: * Improved commit message (thanks Robin!); * Fixed comment style. arch/arm64/kernel/setup.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)