linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [syzbot] upstream-arm64 build error
@ 2023-02-17 10:39 syzbot
  2023-02-20 13:07 ` Mark Rutland
  0 siblings, 1 reply; 6+ messages in thread
From: syzbot @ 2023-02-17 10:39 UTC (permalink / raw)
  To: catalin.marinas, linux-arm-kernel, linux-kernel, syzkaller-bugs, will

Hello,

syzbot found the following issue on:

HEAD commit:    2d3827b3f393 Merge branch 'for-next/core' into for-kernelci
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=160f19d7480000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f5c7f0c5a0c5dbdb
dashboard link: https://syzkaller.appspot.com/bug?extid=f8ac312e31226e23302b
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm64

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f8ac312e31226e23302b@syzkaller.appspotmail.com

failed to run ["make" "-j" "64" "ARCH=arm64" "CROSS_COMPILE=aarch64-linux-gnu-" "CC=clang" "Image.gz"]: exit status 2

---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [syzbot] upstream-arm64 build error
  2023-02-17 10:39 [syzbot] upstream-arm64 build error syzbot
@ 2023-02-20 13:07 ` Mark Rutland
  2023-02-20 13:18   ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2023-02-20 13:07 UTC (permalink / raw)
  To: syzbot
  Cc: catalin.marinas, linux-arm-kernel, linux-kernel, syzkaller-bugs,
	will, ardb

On Fri, Feb 17, 2023 at 02:39:55AM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    2d3827b3f393 Merge branch 'for-next/core' into for-kernelci
> git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
> console output: https://syzkaller.appspot.com/x/log.txt?x=160f19d7480000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=f5c7f0c5a0c5dbdb
> dashboard link: https://syzkaller.appspot.com/bug?extid=f8ac312e31226e23302b
> compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> userspace arch: arm64
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+f8ac312e31226e23302b@syzkaller.appspotmail.com
> 
> failed to run ["make" "-j" "64" "ARCH=arm64" "CROSS_COMPILE=aarch64-linux-gnu-" "CC=clang" "Image.gz"]: exit status 2

For the benefit of others, the actual error from the console log is:

  LD      .tmp_vmlinux.kallsyms1
  aarch64-linux-gnu-ld: ID map text too big or misaligned
  make[1]: *** [scripts/Makefile.vmlinux:35: vmlinux] Error 1
  make: *** [Makefile:1264: vmlinux] Error 2

... and I see the same on for-next/core with my usual fuzzing configs applied
atop, building with GCC 12.1.0.

That "ID map text too big or misalignes" error is from an assertion in arm64's
vmlinux.lds.S, and if I hack that out, the kernel builds and the idmap text
section is 4K aligned and ~2900 bytes in size.

My config worked on v6.2-rc3, and bisecting led me to commit:

  3dcf60bbfd284e5e ("arm64: head: Clean the ID map and the HYP text to the PoC if needed")

... which plays with sections a bit, but doesn't do anything obviously wrong.

I think the error is misleading, and what's actually happening here is that the
size of the .idmap.text section hasn't been determined at the point the
assertion is tested.

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.

Thanks,
Mark.

---->8----
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 212d93aca5e61..13d6f1018eaa1 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -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
@@ -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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [syzbot] upstream-arm64 build error
  2023-02-20 13:07 ` Mark Rutland
@ 2023-02-20 13:18   ` Ard Biesheuvel
  2023-02-20 15:13     ` Mark Rutland
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-02-20 13:18 UTC (permalink / raw)
  To: Mark Rutland
  Cc: syzbot, catalin.marinas, linux-arm-kernel, linux-kernel,
	syzkaller-bugs, will

On Mon, 20 Feb 2023 at 14:07, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Fri, Feb 17, 2023 at 02:39:55AM -0800, syzbot wrote:
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    2d3827b3f393 Merge branch 'for-next/core' into for-kernelci
> > git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
> > console output: https://syzkaller.appspot.com/x/log.txt?x=160f19d7480000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=f5c7f0c5a0c5dbdb
> > dashboard link: https://syzkaller.appspot.com/bug?extid=f8ac312e31226e23302b
> > compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> > userspace arch: arm64
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+f8ac312e31226e23302b@syzkaller.appspotmail.com
> >
> > failed to run ["make" "-j" "64" "ARCH=arm64" "CROSS_COMPILE=aarch64-linux-gnu-" "CC=clang" "Image.gz"]: exit status 2
>
> For the benefit of others, the actual error from the console log is:
>
>   LD      .tmp_vmlinux.kallsyms1
>   aarch64-linux-gnu-ld: ID map text too big or misaligned
>   make[1]: *** [scripts/Makefile.vmlinux:35: vmlinux] Error 1
>   make: *** [Makefile:1264: vmlinux] Error 2
>
> ... and I see the same on for-next/core with my usual fuzzing configs applied
> atop, building with GCC 12.1.0.
>
> That "ID map text too big or misalignes" error is from an assertion in arm64's
> vmlinux.lds.S, and if I hack that out, the kernel builds and the idmap text
> section is 4K aligned and ~2900 bytes in size.
>
> My config worked on v6.2-rc3, and bisecting led me to commit:
>
>   3dcf60bbfd284e5e ("arm64: head: Clean the ID map and the HYP text to the PoC if needed")
>
> ... which plays with sections a bit, but doesn't do anything obviously wrong.
>
> I think the error is misleading, and what's actually happening here is that the
> size of the .idmap.text section hasn't been determined at the point the
> assertion is tested.
>
> 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.

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.

Not sure what the most robust fix would be here - we could try whether
or not placing *.text.stub input sections explicitly makes a
difference here but it still feels a bit ad-hoc (surprisingly,
.idmap.text.stub does not get flagged as an orphan input section even
though it is not mentioned in the linker script)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [syzbot] upstream-arm64 build error
  2023-02-20 13:18   ` Ard Biesheuvel
@ 2023-02-20 15:13     ` Mark Rutland
  2023-02-20 15:38       ` Ard Biesheuvel
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Rutland @ 2023-02-20 15:13 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: syzbot, catalin.marinas, linux-arm-kernel, linux-kernel,
	syzkaller-bugs, will

Hi Ard,

I'm likely missing/confusing terminology below, so apologies in advance for
that!

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@arm.com> wrote:
> >
> > On Fri, Feb 17, 2023 at 02:39:55AM -0800, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:    2d3827b3f393 Merge branch 'for-next/core' into for-kernelci
> > > git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=160f19d7480000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=f5c7f0c5a0c5dbdb
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=f8ac312e31226e23302b
> > > compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> > > userspace arch: arm64
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+f8ac312e31226e23302b@syzkaller.appspotmail.com
> > >
> > > failed to run ["make" "-j" "64" "ARCH=arm64" "CROSS_COMPILE=aarch64-linux-gnu-" "CC=clang" "Image.gz"]: exit status 2
> >
> > For the benefit of others, the actual error from the console log is:
> >
> >   LD      .tmp_vmlinux.kallsyms1
> >   aarch64-linux-gnu-ld: ID map text too big or misaligned
> >   make[1]: *** [scripts/Makefile.vmlinux:35: vmlinux] Error 1
> >   make: *** [Makefile:1264: vmlinux] Error 2
> >
> > ... and I see the same on for-next/core with my usual fuzzing configs applied
> > atop, building with GCC 12.1.0.
> >
> > That "ID map text too big or misalignes" error is from an assertion in arm64's
> > vmlinux.lds.S, and if I hack that out, the kernel builds and the idmap text
> > section is 4K aligned and ~2900 bytes in size.
> >
> > My config worked on v6.2-rc3, and bisecting led me to commit:
> >
> >   3dcf60bbfd284e5e ("arm64: head: Clean the ID map and the HYP text to the PoC if needed")
> >
> > ... which plays with sections a bit, but doesn't do anything obviously wrong.
> >
> > I think the error is misleading, and what's actually happening here is that the
> > size of the .idmap.text section hasn't been determined at the point the
> > assertion is tested.
> >
> > 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* ?

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.

FWIW, If I modify head.S to replace:

  .section ".idmap.text","a"

With:

  .section ".idmap.text","a"

Then the only branches which don't get PLTs are the ones I mentioned earlier,
and the linker complains about those becoming truncated:

| [mark@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?

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [syzbot] upstream-arm64 build error
  2023-02-20 15:13     ` Mark Rutland
@ 2023-02-20 15:38       ` Ard Biesheuvel
  2023-02-20 15:56         ` Mark Rutland
  0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2023-02-20 15:38 UTC (permalink / raw)
  To: Mark Rutland
  Cc: syzbot, catalin.marinas, linux-arm-kernel, linux-kernel,
	syzkaller-bugs, will

On Mon, 20 Feb 2023 at 16:13, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Ard,
>
> I'm likely missing/confusing terminology below, so apologies in advance for
> that!
>
> 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@arm.com> wrote:
> > >
> > > On Fri, Feb 17, 2023 at 02:39:55AM -0800, syzbot wrote:
> > > > Hello,
> > > >
> > > > syzbot found the following issue on:
> > > >
> > > > HEAD commit:    2d3827b3f393 Merge branch 'for-next/core' into for-kernelci
> > > > git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=160f19d7480000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=f5c7f0c5a0c5dbdb
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f8ac312e31226e23302b
> > > > compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> > > > userspace arch: arm64
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > Reported-by: syzbot+f8ac312e31226e23302b@syzkaller.appspotmail.com
> > > >
> > > > failed to run ["make" "-j" "64" "ARCH=arm64" "CROSS_COMPILE=aarch64-linux-gnu-" "CC=clang" "Image.gz"]: exit status 2
> > >
> > > For the benefit of others, the actual error from the console log is:
> > >
> > >   LD      .tmp_vmlinux.kallsyms1
> > >   aarch64-linux-gnu-ld: ID map text too big or misaligned
> > >   make[1]: *** [scripts/Makefile.vmlinux:35: vmlinux] Error 1
> > >   make: *** [Makefile:1264: vmlinux] Error 2
> > >
> > > ... and I see the same on for-next/core with my usual fuzzing configs applied
> > > atop, building with GCC 12.1.0.
> > >
> > > That "ID map text too big or misalignes" error is from an assertion in arm64's
> > > vmlinux.lds.S, and if I hack that out, the kernel builds and the idmap text
> > > section is 4K aligned and ~2900 bytes in size.
> > >
> > > My config worked on v6.2-rc3, and bisecting led me to commit:
> > >
> > >   3dcf60bbfd284e5e ("arm64: head: Clean the ID map and the HYP text to the PoC if needed")
> > >
> > > ... which plays with sections a bit, but doesn't do anything obviously wrong.
> > >
> > > I think the error is misleading, and what's actually happening here is that the
> > > size of the .idmap.text section hasn't been determined at the point the
> > > assertion is tested.
> > >
> > > 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.

> 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.

> 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.

> Then the only branches which don't get PLTs are the ones I mentioned earlier,
> and the linker complains about those becoming truncated:
>
> | [mark@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.

> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [syzbot] upstream-arm64 build error
  2023-02-20 15:38       ` Ard Biesheuvel
@ 2023-02-20 15:56         ` Mark Rutland
  0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2023-02-20 15:56 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: syzbot, catalin.marinas, linux-arm-kernel, linux-kernel,
	syzkaller-bugs, will

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@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@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@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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-20 15:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 10:39 [syzbot] upstream-arm64 build error syzbot
2023-02-20 13:07 ` Mark Rutland
2023-02-20 13:18   ` Ard Biesheuvel
2023-02-20 15:13     ` Mark Rutland
2023-02-20 15:38       ` Ard Biesheuvel
2023-02-20 15:56         ` Mark Rutland

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).