[syzbot] upstream-arm64 build error
Mark Rutland
mark.rutland at arm.com
Mon Feb 20 07:56:50 PST 2023
On Mon, Feb 20, 2023 at 04:38:31PM +0100, Ard Biesheuvel wrote:
> On Mon, 20 Feb 2023 at 16:13, Mark Rutland <mark.rutland at arm.com> wrote:
> > On Mon, Feb 20, 2023 at 02:18:23PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 20 Feb 2023 at 14:07, Mark Rutland <mark.rutland at arm.com> wrote:
> > > > With my config, the Image size is ~242MiB, and I think what's happening is that
> > > > some branches from .idmap.text to .text are (possibly) out-of-range, but the
> > > > linker doesn't know the final position of the sections yet and hasn't placed
> > > > PLTs, and doesn't know the final size of the sections.
> > > >
> > > > I don't know much about the linker, so that's conjecture, but the below diff
> > > > got rid of the build error for me.
> > >
> > > This issue was reported before here:
> > >
> > > https://lore.kernel.org/all/CAMj1kXGAf7ikEU5jLoik0xrOde0xBg0yJkOo5=PtEtNXoUxMXA@mail.gmail.com/
> > >
> > > and the bisect ended up somewhere else.
> >
> > Ah, sorry; I hadn't spotted that.
> >
> > > The issue seems to be where exactly the veneers for the entire image
> > > are dumped, and when this is right after .idmap.text (being the last
> > > input section with the EXEC ELF attribute), it pushes the
> > > __idmap_text_end symbol over the next 4k boundary.
> >
> > I'm a bit confused as to why veneers for other sections would be dropped in
> > here. Note that these branches (for-next/core and for-kernelci) have your patch
> > moving IDMAP_TEXT into .rodata, so that's no longer the last input section with
> > the EXEC attribute, unless I've misunderstood?
> >
> > Perhaps that's meant to be read as the last input section *within a given
> > output section* ?
>
> The issue seems subtly different in this case:
>
> .idmap.text 0xffffffc01e48e5c0 0x32c arch/arm64/mm/proc.o
> 0xffffffc01e48e5c0 idmap_cpu_replace_ttbr1
> 0xffffffc01e48e600 idmap_kpti_install_ng_mappings
> 0xffffffc01e48e800 __cpu_setup
> *fill* 0xffffffc01e48e8ec 0x4
> .idmap.text.stub
> 0xffffffc01e48e8f0 0x18 linker stubs
> 0xffffffc01e48f8f0 __idmap_text_end = .
> 0xffffffc01e48f000 . = ALIGN (0x1000)
> *fill* 0xffffffc01e48f8f0 0x710
> 0xffffffc01e490000 idmap_pg_dir = .
>
> So here, there is 0x18 bytes of stubs, but for some reason, it ends up
> bumping the output pointer by an additional 4k.
Yeah, the 4K bump is bizarre, especially since it's literally +4K and not an
align upwards to the next 4K boundary.
> > If marking the .idmap.text section as non-executable prevents other veneers
> > from being collected there, then I reckon that doing that along with the
> > indirect branches using adr_l + blr should be sufficient? That way there should
> > be no veneers dropped into .idmap.text, and the cost of ADRP+ADD+BLR in a
> > one-time boot path isn't going to be noticeable.
> >
>
> I guess but I hate making that kind of changes just to make a
> theoretical boot target that nobody uses build.
I appreciate it's niche, but this is a configuration I've been using for
testing for several years (see my fuzzing/* and testing/* branches on
kernel.org), so it's not quite "nobody" unless that was an a judgement on my
popularity. ;)
I'm going to assume that means if I clean this up and post as a proper patch
you're not going to NAK it. :)
> > FWIW, If I modify head.S to replace:
> >
> > .section ".idmap.text","a"
> >
> > With:
> >
> > .section ".idmap.text","a"
> >
>
> You mean ax -> a right? That seems like a reasonable change as long as
> we apply it everywhere, but it would be nice if we wouldn't need it.
Yes, sorry, It's currently "awx", and AFAICT we need neither the "w" nor the
"x".
> > Then the only branches which don't get PLTs are the ones I mentioned earlier,
> > and the linker complains about those becoming truncated:
> >
> > | [mark at lakrids:~/src/linux]% usekorg 12.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -j50 Image
> > | CALL scripts/checksyscalls.sh
> > | LDS arch/arm64/kernel/vmlinux.lds
> > | AS arch/arm64/kernel/head.o
> > | AR arch/arm64/kernel/built-in.a
> > | AR arch/arm64/built-in.a
> > | AR built-in.a
> > | AR vmlinux.a
> > | LD vmlinux.o
> > | OBJCOPY modules.builtin.modinfo
> > | GEN modules.builtin
> > | MODPOST vmlinux.symvers
> > | UPD include/generated/utsversion.h
> > | CC init/version-timestamp.o
> > | LD .tmp_vmlinux.kallsyms1
> > | arch/arm64/kernel/head.o: in function `primary_entry':
> > | (.idmap.text+0x1c): relocation truncated to fit: R_AARCH64_CALL26 against symbol `dcache_clean_poc' defined in .text section in arch/arm64/mm/cache.o
> > | arch/arm64/kernel/head.o: in function `init_el2':
> > | (.idmap.text+0x88): relocation truncated to fit: R_AARCH64_CALL26 against symbol `dcache_clean_poc' defined in .text section in arch/arm64/mm/cache.o
> > | make[1]: *** [scripts/Makefile.vmlinux:34: vmlinux] Error 1
> > | make: *** [Makefile:1252: vmlinux] Error 2
> >
> > Is there any reason we need the "wx" attributs for .idmap.text?
>
> It doesn't really matter - we place these input sections explicitly anyway.
That was my assumption; just checking I haven't missed a subtlety where this
would affect assembler or linker behaviour undesireably.
Thanks,
Mark.
> > With that in mind, does the below look ok? It builds cleanly and boots fine for
> > me with GCC 12.1.0 atop for-next/core.
> >
> > We could wrap the adr_l + blr into a blr_l macro for legibility.
> >
> > Thanks,
> > Mark.
> >
> > ---->8----
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 212d93aca5e61..b98970907226b 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -70,7 +70,7 @@
> >
> > __EFI_PE_HEADER
> >
> > - .section ".idmap.text","awx"
> > + .section ".idmap.text","a"
> >
> > /*
> > * The following callee saved general purpose registers are used on the
> > @@ -99,7 +99,8 @@ SYM_CODE_START(primary_entry)
> > cbz x19, 0f
> > adrp x0, __idmap_text_start
> > adr_l x1, __idmap_text_end
> > - bl dcache_clean_poc
> > + adr_l x2, dcache_clean_poc
> > + blr x2
> > 0: mov x0, x19
> > bl init_kernel_el // w0=cpu_boot_mode
> > mov x20, x0
> > @@ -527,7 +528,7 @@ SYM_FUNC_END(__primary_switched)
> > * end early head section, begin head code that is also used for
> > * hotplug and needs to have the same protections as the text region
> > */
> > - .section ".idmap.text","awx"
> > + .section ".idmap.text","a"
> >
> > /*
> > * Starting from EL2 or EL1, configure the CPU to execute at the highest
> > @@ -566,7 +567,8 @@ SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
> > cbz x0, 0f
> > adrp x0, __hyp_idmap_text_start
> > adr_l x1, __hyp_text_end
> > - bl dcache_clean_poc
> > + adr_l x2, dcache_clean_poc
> > + blr x2
> > 0:
> > mov_q x0, HCR_HOST_NVHE_FLAGS
> > msr hcr_el2, x0
> > @@ -728,7 +730,7 @@ SYM_FUNC_END(set_cpu_boot_mode_flag)
> > * Checks if the selected granule size is supported by the CPU.
> > * If it isn't, park the CPU
> > */
> > - .section ".idmap.text","awx"
> > + .section ".idmap.text","a"
> > SYM_FUNC_START(__enable_mmu)
> > mrs x3, ID_AA64MMFR0_EL1
> > ubfx x3, x3, #ID_AA64MMFR0_EL1_TGRAN_SHIFT, 4
More information about the linux-arm-kernel
mailing list