* [PATCH 1/5] bfd: aarch64: Fix BTI stub optimization PR30957
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
@ 2023-11-03 13:15 ` Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 2/5] bfd: aarch64: Fix broken BTI stub PR30930 Szabolcs Nagy
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:15 UTC (permalink / raw)
To: binutils
The instruction was looked up in the wrong input file (file of branch
source instead of branch target) when optimizing away BTI stubs in
commit 5834f36d93cabf1a8bcc7dd7654141aed3d296bc
bfd: aarch64: Optimize BTI stubs PR30076
This can cause adding BTI stubs when they are not necessary or removing
them when they are (the latter is a correctness issue but it is very
unlikely in practice).
Fixes bug 30957.
---
bfd/elfnn-aarch64.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 4592bd6da27..a0dd17faa7e 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -3675,8 +3675,7 @@ group_sections (struct elf_aarch64_link_hash_table *htab,
/* True if the inserted stub does not break BTI compatibility. */
static bool
-aarch64_bti_stub_p (bfd *input_bfd,
- struct elf_aarch64_stub_hash_entry *stub_entry)
+aarch64_bti_stub_p (struct elf_aarch64_stub_hash_entry *stub_entry)
{
/* Stubs without indirect branch are BTI compatible. */
if (stub_entry->stub_type != aarch64_stub_adrp_branch
@@ -3690,7 +3689,7 @@ aarch64_bti_stub_p (bfd *input_bfd,
file_ptr off = stub_entry->target_value;
bfd_size_type count = sizeof (loc);
- if (!bfd_get_section_contents (input_bfd, section, loc, off, count))
+ if (!bfd_get_section_contents (section->owner, section, loc, off, count))
return false;
uint32_t insn = bfd_getl32 (loc);
@@ -4637,7 +4636,7 @@ _bfd_aarch64_add_call_stub_entries (bool *stub_changed, bfd *output_bfd,
/* A stub with indirect jump may break BTI compatibility, so
insert another stub with direct jump near the target then. */
- if (need_bti && !aarch64_bti_stub_p (input_bfd, stub_entry))
+ if (need_bti && !aarch64_bti_stub_p (stub_entry))
{
stub_entry->double_stub = true;
htab->has_double_stub = true;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 2/5] bfd: aarch64: Fix broken BTI stub PR30930
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 1/5] bfd: aarch64: Fix BTI stub optimization PR30957 Szabolcs Nagy
@ 2023-11-03 13:15 ` Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 3/5] bfd: aarch64: Fix leaks in case of BTI stub reuse Szabolcs Nagy
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:15 UTC (permalink / raw)
To: binutils
Input sections are grouped together that can use the same stub area
(within reach) and these groups have a stable id.
Stubs have a name generated from the stub group id and target symbol.
When a relocation requires a stub with a name that already exists, the
stub is reused instead of adding a new one.
For an indirect branch stub another BTI stub may be inserted near the
target to provide a BTI landing pad.
The BTI stub can end up with the same stub group id and thus the same
name as the indirect stub. This happens if the target symbol is within
reach of the indirect branch stub. Then, due to the name collision,
only a single stub was emmitted which branched to itself causing an
infinite loop at runtime.
A possible solution is to just name the BTI stubs differently, but
since in the problematic case the indirect and BTI stub are in the
same stub area, a better solution is to emit a single stub with a
direct branch. The stub is still needed since the caller cannot reach
the target directly and we also want a BTI landing pad in the stub in
case other indirect stubs target the same symbol and thus need a BTI
stub.
In short we convert an indirect branch stub into a BTI stub when the
target is within reach and has no BTI. It is a hassle to change the
symbol of the stub so a BTI stub may end up with *_veneer instead of
*_bti_veneer after the conversion, but this should not matter much.
(Refactoring some of _bfd_aarch64_add_call_stub_entries would be
useful but too much for this bug fix patch.)
The same conversion to direct branch could be done even if the target
did not need a BTI. The stub groups are fixed in the current logic so
linking can fail if too many stubs are inserted and the section layout
is changed too much, but this only happens in extreme cases that can
be reasonably ignored. Because of this the target cannot go out of
reach during stub insertion so the optimization is valid, but not
implemented by this patch for the non-BTI case.
Fixes bug 30930.
---
bfd/elfnn-aarch64.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index a0dd17faa7e..798643ade7c 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -4638,9 +4638,22 @@ _bfd_aarch64_add_call_stub_entries (bool *stub_changed, bfd *output_bfd,
insert another stub with direct jump near the target then. */
if (need_bti && !aarch64_bti_stub_p (stub_entry))
{
+ id_sec_bti = htab->stub_group[sym_sec->id].link_sec;
+
+ /* If the stub with indirect jump and the BTI stub are in
+ the same stub group: change the indirect jump stub into
+ a BTI stub since a direct branch can reach the target.
+ The BTI landing pad is still needed in case another
+ stub indirectly jumps to it. */
+ if (id_sec_bti == id_sec)
+ {
+ stub_entry->stub_type = aarch64_stub_bti_direct_branch;
+ goto skip_double_stub;
+ }
+
stub_entry->double_stub = true;
htab->has_double_stub = true;
- id_sec_bti = htab->stub_group[sym_sec->id].link_sec;
+
stub_name_bti =
elfNN_aarch64_stub_name (id_sec_bti, sym_sec, hash, irela);
if (!stub_name_bti)
@@ -4687,7 +4700,7 @@ _bfd_aarch64_add_call_stub_entries (bool *stub_changed, bfd *output_bfd,
stub_entry->h = NULL;
stub_entry->st_type = STT_FUNC;
}
-
+skip_double_stub:
*stub_changed = true;
}
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 3/5] bfd: aarch64: Fix leaks in case of BTI stub reuse
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 1/5] bfd: aarch64: Fix BTI stub optimization PR30957 Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 2/5] bfd: aarch64: Fix broken BTI stub PR30930 Szabolcs Nagy
@ 2023-11-03 13:15 ` Szabolcs Nagy
2023-11-03 13:15 ` [PATCH 4/5] bfd: aarch64: Avoid BTI stub for a PLT that has BTI Szabolcs Nagy
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:15 UTC (permalink / raw)
To: binutils
BTI stub parameters were recomputed even if those were already set up.
This is unnecessary work and leaks the symbol name that is allocated
for the stub.
---
bfd/elfnn-aarch64.c | 56 ++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 798643ade7c..3adece30250 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -4665,33 +4665,41 @@ _bfd_aarch64_add_call_stub_entries (bool *stub_changed, bfd *output_bfd,
stub_entry_bti =
aarch64_stub_hash_lookup (&htab->stub_hash_table,
stub_name_bti, false, false);
- if (stub_entry_bti == NULL)
- stub_entry_bti =
- _bfd_aarch64_add_stub_entry_in_group (stub_name_bti,
- sym_sec, htab);
- if (stub_entry_bti == NULL)
+ if (stub_entry_bti != NULL)
+ BFD_ASSERT (stub_entry_bti->stub_type
+ == aarch64_stub_bti_direct_branch);
+ else
{
- free (stub_name);
- free (stub_name_bti);
- goto error_ret_free_internal;
- }
-
- stub_entry_bti->target_value = sym_value + irela->r_addend;
- stub_entry_bti->target_section = sym_sec;
- stub_entry_bti->stub_type = aarch64_stub_bti_direct_branch;
- stub_entry_bti->h = hash;
- stub_entry_bti->st_type = st_type;
+ stub_entry_bti =
+ _bfd_aarch64_add_stub_entry_in_group (stub_name_bti,
+ sym_sec, htab);
+ if (stub_entry_bti == NULL)
+ {
+ free (stub_name);
+ free (stub_name_bti);
+ goto error_ret_free_internal;
+ }
- len = sizeof (BTI_STUB_ENTRY_NAME) + strlen (sym_name);
- stub_entry_bti->output_name = bfd_alloc (htab->stub_bfd, len);
- if (stub_entry_bti->output_name == NULL)
- {
- free (stub_name);
- free (stub_name_bti);
- goto error_ret_free_internal;
+ stub_entry_bti->target_value =
+ sym_value + irela->r_addend;
+ stub_entry_bti->target_section = sym_sec;
+ stub_entry_bti->stub_type =
+ aarch64_stub_bti_direct_branch;
+ stub_entry_bti->h = hash;
+ stub_entry_bti->st_type = st_type;
+
+ len = sizeof (BTI_STUB_ENTRY_NAME) + strlen (sym_name);
+ stub_entry_bti->output_name = bfd_alloc (htab->stub_bfd,
+ len);
+ if (stub_entry_bti->output_name == NULL)
+ {
+ free (stub_name);
+ free (stub_name_bti);
+ goto error_ret_free_internal;
+ }
+ snprintf (stub_entry_bti->output_name, len,
+ BTI_STUB_ENTRY_NAME, sym_name);
}
- snprintf (stub_entry_bti->output_name, len,
- BTI_STUB_ENTRY_NAME, sym_name);
/* Update the indirect call stub to target the BTI stub. */
stub_entry->target_value = 0;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 4/5] bfd: aarch64: Avoid BTI stub for a PLT that has BTI
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
` (2 preceding siblings ...)
2023-11-03 13:15 ` [PATCH 3/5] bfd: aarch64: Fix leaks in case of BTI stub reuse Szabolcs Nagy
@ 2023-11-03 13:15 ` Szabolcs Nagy
2023-11-03 13:22 ` [PATCH 5/5] ld: aarch64: Add BTI stub insertion test PR30930 Szabolcs Nagy
2023-11-07 11:38 ` [PATCH 0/5] aarch64 BTI stub fixes Nick Clifton
5 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:15 UTC (permalink / raw)
To: binutils
We decide to emit BTI stubs based on the instruction at the target
location. But PLT code is generated later than the stubs so we always
read 0 which is not a valid BTI.
Fix the logic to special case the PLT section: this is code the linker
generates so we know when it will have BTI.
This avoids BTI stubs in large executables where the PLTs have them
already. An alternative is to never emit BTI stubs for PLTs, instead
use BTI in the PLT if a library gets too big, however that may be
more tricky given the ordering of PLT sizing and stub insertion.
Related to bug 30957.
---
bfd/elfnn-aarch64.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 3adece30250..4faf642b422 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -3675,7 +3675,8 @@ group_sections (struct elf_aarch64_link_hash_table *htab,
/* True if the inserted stub does not break BTI compatibility. */
static bool
-aarch64_bti_stub_p (struct elf_aarch64_stub_hash_entry *stub_entry)
+aarch64_bti_stub_p (struct bfd_link_info *info,
+ struct elf_aarch64_stub_hash_entry *stub_entry)
{
/* Stubs without indirect branch are BTI compatible. */
if (stub_entry->stub_type != aarch64_stub_adrp_branch
@@ -3684,12 +3685,22 @@ aarch64_bti_stub_p (struct elf_aarch64_stub_hash_entry *stub_entry)
/* Return true if the target instruction is compatible with BR x16. */
+ struct elf_aarch64_link_hash_table *globals = elf_aarch64_hash_table (info);
asection *section = stub_entry->target_section;
bfd_byte loc[4];
file_ptr off = stub_entry->target_value;
bfd_size_type count = sizeof (loc);
- if (!bfd_get_section_contents (section->owner, section, loc, off, count))
+ /* PLT code is not generated yet, so treat it specially.
+ Note: Checking elf_aarch64_obj_tdata.plt_type & PLT_BTI is not
+ enough because it only implies BTI in the PLT0 and tlsdesc PLT
+ entries. Normal PLT entries don't have BTI in a shared library
+ (because such PLT is normally not called indirectly and adding
+ the BTI when a stub targets a PLT would change the PLT layout
+ and it's too late for that here). */
+ if (section == globals->root.splt)
+ memcpy (loc, globals->plt_entry, count);
+ else if (!bfd_get_section_contents (section->owner, section, loc, off, count))
return false;
uint32_t insn = bfd_getl32 (loc);
@@ -4636,7 +4647,7 @@ _bfd_aarch64_add_call_stub_entries (bool *stub_changed, bfd *output_bfd,
/* A stub with indirect jump may break BTI compatibility, so
insert another stub with direct jump near the target then. */
- if (need_bti && !aarch64_bti_stub_p (stub_entry))
+ if (need_bti && !aarch64_bti_stub_p (info, stub_entry))
{
id_sec_bti = htab->stub_group[sym_sec->id].link_sec;
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 5/5] ld: aarch64: Add BTI stub insertion test PR30930
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
` (3 preceding siblings ...)
2023-11-03 13:15 ` [PATCH 4/5] bfd: aarch64: Avoid BTI stub for a PLT that has BTI Szabolcs Nagy
@ 2023-11-03 13:22 ` Szabolcs Nagy
2023-11-03 13:15 ` Szabolcs Nagy
2023-11-07 11:38 ` [PATCH 0/5] aarch64 BTI stub fixes Nick Clifton
5 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:22 UTC (permalink / raw)
To: binutils
The test creates a large shared library and covers a number of
BTI stub insertion cases.
---
ld/testsuite/ld-aarch64/aarch64-elf.exp | 1 +
ld/testsuite/ld-aarch64/bti-far-3.d | 87 +++++++++++++++++++++++++
ld/testsuite/ld-aarch64/bti-far-3.ld | 12 ++++
ld/testsuite/ld-aarch64/bti-far-3a.s | 23 +++++++
ld/testsuite/ld-aarch64/bti-far-3b.s | 25 +++++++
ld/testsuite/ld-aarch64/bti-far-3c.s | 24 +++++++
6 files changed, 172 insertions(+)
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.d
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.ld
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3a.s
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3b.s
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3c.s
diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp
index b025fcbd567..fa5ac3a2cdd 100644
--- a/ld/testsuite/ld-aarch64/aarch64-elf.exp
+++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp
@@ -466,6 +466,7 @@ run_dump_test "undef-tls"
run_dump_test "bti-far-1"
run_dump_test "bti-far-2"
run_dump_test "bti-far-opt"
+run_dump_test "bti-far-3"
if { ![skip_sframe_tests] } {
run_dump_test "sframe-simple-1"
diff --git a/ld/testsuite/ld-aarch64/bti-far-3.d b/ld/testsuite/ld-aarch64/bti-far-3.d
new file mode 100644
index 00000000000..4c076985a0e
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3.d
@@ -0,0 +1,87 @@
+#name: Check linker stubs with indirect calls handle BTI (exe).
+#source: bti-far-3a.s
+#source: bti-far-3b.s
+#source: bti-far-3c.s
+#as: -mabi=lp64
+#ld: -shared -T bti-far-3.ld
+#objdump: -dr
+
+[^:]*: *file format elf64-.*aarch64
+
+
+Disassembly of section \.plt:
+
+0000000000020000 <\.plt>:
+ 20000: d503245f bti c
+ 20004: a9bf7bf0 stp x16, x30, \[sp, #-16\]!
+ 20008: 900fff10 adrp x16, 20000000 <_GLOBAL_OFFSET_TABLE_>
+ 2000c: f9400e11 ldr x17, \[x16, #24\]
+ 20010: 91006210 add x16, x16, #0x18
+ 20014: d61f0220 br x17
+ 20018: d503201f nop
+ 2001c: d503201f nop
+
+0000000000020020 <extern_func@plt>:
+ 20020: 900fff10 adrp x16, 20000000 <_GLOBAL_OFFSET_TABLE_>
+ 20024: f9401211 ldr x17, \[x16, #32\]
+ 20028: 91008210 add x16, x16, #0x20
+ 2002c: d61f0220 br x17
+ 20030: 14000004 b 20040 <__extern_func_bti_veneer\+0x8>
+ 20034: d503201f nop
+
+0000000000020038 <__extern_func_bti_veneer>:
+ 20038: d503245f bti c
+ 2003c: 17fffff9 b 20020 <extern_func@plt>
+
+Disassembly of section \.text:
+
+0000000000030000 <a_func>:
+ 30000: 15c00004 b 7030010 <__b_func_veneer>
+ 30004: 17ffc007 b 20020 <extern_func@plt>
+ \.\.\.
+ 7030008: 1400000a b 7030030 <__a_func_bti_veneer\+0x8>
+ 703000c: d503201f nop
+
+0000000007030010 <__b_func_veneer>:
+ 7030010: 90040010 adrp x16, f030000 <b_func\+0x6ffffd0>
+ 7030014: 9101e210 add x16, x16, #0x78
+ 7030018: d61f0200 br x16
+ \.\.\.
+
+0000000007030028 <__a_func_bti_veneer>:
+ 7030028: d503245f bti c
+ 703002c: 163ffff5 b 30000 <a_func>
+ \.\.\.
+
+0000000008030030 <b_func>:
+ 8030030: 15c00004 b f030040 <__c_func_veneer>
+ 8030034: 15c00005 b f030048 <__a_func_veneer>
+ \.\.\.
+ f030038: 14000012 b f030080 <__b_func_bti_veneer\+0x8>
+ f03003c: d503201f nop
+
+000000000f030040 <__c_func_veneer>:
+ f030040: d503245f bti c
+ f030044: 1440000f b 10030080 <c_func>
+
+000000000f030048 <__a_func_veneer>:
+ f030048: 90fc0010 adrp x16, 7030000 <a_func\+0x7000000>
+ f03004c: 9100a210 add x16, x16, #0x28
+ f030050: d61f0200 br x16
+ \.\.\.
+
+000000000f030060 <__extern_func_veneer>:
+ f030060: 90f87f90 adrp x16, 20000 <\.plt>
+ f030064: 9100e210 add x16, x16, #0x38
+ f030068: d61f0200 br x16
+ \.\.\.
+
+000000000f030078 <__b_func_bti_veneer>:
+ f030078: d503245f bti c
+ f03007c: 163fffed b 8030030 <b_func>
+ \.\.\.
+
+0000000010030080 <c_func>:
+ 10030080: 17bffff2 b f030048 <__a_func_veneer>
+ 10030084: 17bffffd b f030078 <__b_func_bti_veneer>
+ 10030088: 17bffff6 b f030060 <__extern_func_veneer>
diff --git a/ld/testsuite/ld-aarch64/bti-far-3.ld b/ld/testsuite/ld-aarch64/bti-far-3.ld
new file mode 100644
index 00000000000..3265c55b8cc
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3.ld
@@ -0,0 +1,12 @@
+OUTPUT_ARCH(aarch64)
+SECTIONS
+{
+ . = 0x00010000;
+ .rela.plt : { *(.rela.plt) *(.rela.iplt) }
+ . = 0x00020000;
+ .plt : { *(.plt) *(.iplt) }
+ . = 0x00030000;
+ .text : { *(.text) }
+ . = 0x20000000;
+ .got : { *(.got) *(.got.plt) }
+}
diff --git a/ld/testsuite/ld-aarch64/bti-far-3a.s b/ld/testsuite/ld-aarch64/bti-far-3a.s
new file mode 100644
index 00000000000..ed53545fed7
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3a.s
@@ -0,0 +1,23 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+ .global a_func
+ .type a_func, %function
+a_func:
+ b b_func
+ b extern_func
+
+.zero 0x07000000
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
diff --git a/ld/testsuite/ld-aarch64/bti-far-3b.s b/ld/testsuite/ld-aarch64/bti-far-3b.s
new file mode 100644
index 00000000000..1f3be3d15cf
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3b.s
@@ -0,0 +1,25 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+.zero 0x01000000
+
+ .global b_func
+ .type b_func, %function
+b_func:
+ b c_func
+ b a_func
+
+.zero 0x07000000
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
diff --git a/ld/testsuite/ld-aarch64/bti-far-3c.s b/ld/testsuite/ld-aarch64/bti-far-3c.s
new file mode 100644
index 00000000000..8e2361fd771
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3c.s
@@ -0,0 +1,24 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+.zero 0x01000000
+
+ .global c_func
+ .type c_func, %function
+c_func:
+ b a_func
+ b b_func
+ b extern_func
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 5/5] ld: aarch64: Add BTI stub insertion test PR30930
2023-11-03 13:22 ` [PATCH 5/5] ld: aarch64: Add BTI stub insertion test PR30930 Szabolcs Nagy
@ 2023-11-03 13:15 ` Szabolcs Nagy
0 siblings, 0 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-03 13:15 UTC (permalink / raw)
To: binutils
The test creates a large shared library and covers a number of
BTI stub insertion cases.
---
ld/testsuite/ld-aarch64/aarch64-elf.exp | 1 +
ld/testsuite/ld-aarch64/bti-far-3.d | 87 +++++++++++++++++++++++++
ld/testsuite/ld-aarch64/bti-far-3.ld | 12 ++++
ld/testsuite/ld-aarch64/bti-far-3a.s | 23 +++++++
ld/testsuite/ld-aarch64/bti-far-3b.s | 25 +++++++
ld/testsuite/ld-aarch64/bti-far-3c.s | 24 +++++++
6 files changed, 172 insertions(+)
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.d
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3.ld
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3a.s
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3b.s
create mode 100644 ld/testsuite/ld-aarch64/bti-far-3c.s
diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp
index b025fcbd567..fa5ac3a2cdd 100644
--- a/ld/testsuite/ld-aarch64/aarch64-elf.exp
+++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp
@@ -466,6 +466,7 @@ run_dump_test "undef-tls"
run_dump_test "bti-far-1"
run_dump_test "bti-far-2"
run_dump_test "bti-far-opt"
+run_dump_test "bti-far-3"
if { ![skip_sframe_tests] } {
run_dump_test "sframe-simple-1"
diff --git a/ld/testsuite/ld-aarch64/bti-far-3.d b/ld/testsuite/ld-aarch64/bti-far-3.d
new file mode 100644
index 00000000000..4c076985a0e
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3.d
@@ -0,0 +1,87 @@
+#name: Check linker stubs with indirect calls handle BTI (exe).
+#source: bti-far-3a.s
+#source: bti-far-3b.s
+#source: bti-far-3c.s
+#as: -mabi=lp64
+#ld: -shared -T bti-far-3.ld
+#objdump: -dr
+
+[^:]*: *file format elf64-.*aarch64
+
+
+Disassembly of section \.plt:
+
+0000000000020000 <\.plt>:
+ 20000: d503245f bti c
+ 20004: a9bf7bf0 stp x16, x30, \[sp, #-16\]!
+ 20008: 900fff10 adrp x16, 20000000 <_GLOBAL_OFFSET_TABLE_>
+ 2000c: f9400e11 ldr x17, \[x16, #24\]
+ 20010: 91006210 add x16, x16, #0x18
+ 20014: d61f0220 br x17
+ 20018: d503201f nop
+ 2001c: d503201f nop
+
+0000000000020020 <extern_func@plt>:
+ 20020: 900fff10 adrp x16, 20000000 <_GLOBAL_OFFSET_TABLE_>
+ 20024: f9401211 ldr x17, \[x16, #32\]
+ 20028: 91008210 add x16, x16, #0x20
+ 2002c: d61f0220 br x17
+ 20030: 14000004 b 20040 <__extern_func_bti_veneer\+0x8>
+ 20034: d503201f nop
+
+0000000000020038 <__extern_func_bti_veneer>:
+ 20038: d503245f bti c
+ 2003c: 17fffff9 b 20020 <extern_func@plt>
+
+Disassembly of section \.text:
+
+0000000000030000 <a_func>:
+ 30000: 15c00004 b 7030010 <__b_func_veneer>
+ 30004: 17ffc007 b 20020 <extern_func@plt>
+ \.\.\.
+ 7030008: 1400000a b 7030030 <__a_func_bti_veneer\+0x8>
+ 703000c: d503201f nop
+
+0000000007030010 <__b_func_veneer>:
+ 7030010: 90040010 adrp x16, f030000 <b_func\+0x6ffffd0>
+ 7030014: 9101e210 add x16, x16, #0x78
+ 7030018: d61f0200 br x16
+ \.\.\.
+
+0000000007030028 <__a_func_bti_veneer>:
+ 7030028: d503245f bti c
+ 703002c: 163ffff5 b 30000 <a_func>
+ \.\.\.
+
+0000000008030030 <b_func>:
+ 8030030: 15c00004 b f030040 <__c_func_veneer>
+ 8030034: 15c00005 b f030048 <__a_func_veneer>
+ \.\.\.
+ f030038: 14000012 b f030080 <__b_func_bti_veneer\+0x8>
+ f03003c: d503201f nop
+
+000000000f030040 <__c_func_veneer>:
+ f030040: d503245f bti c
+ f030044: 1440000f b 10030080 <c_func>
+
+000000000f030048 <__a_func_veneer>:
+ f030048: 90fc0010 adrp x16, 7030000 <a_func\+0x7000000>
+ f03004c: 9100a210 add x16, x16, #0x28
+ f030050: d61f0200 br x16
+ \.\.\.
+
+000000000f030060 <__extern_func_veneer>:
+ f030060: 90f87f90 adrp x16, 20000 <\.plt>
+ f030064: 9100e210 add x16, x16, #0x38
+ f030068: d61f0200 br x16
+ \.\.\.
+
+000000000f030078 <__b_func_bti_veneer>:
+ f030078: d503245f bti c
+ f03007c: 163fffed b 8030030 <b_func>
+ \.\.\.
+
+0000000010030080 <c_func>:
+ 10030080: 17bffff2 b f030048 <__a_func_veneer>
+ 10030084: 17bffffd b f030078 <__b_func_bti_veneer>
+ 10030088: 17bffff6 b f030060 <__extern_func_veneer>
diff --git a/ld/testsuite/ld-aarch64/bti-far-3.ld b/ld/testsuite/ld-aarch64/bti-far-3.ld
new file mode 100644
index 00000000000..3265c55b8cc
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3.ld
@@ -0,0 +1,12 @@
+OUTPUT_ARCH(aarch64)
+SECTIONS
+{
+ . = 0x00010000;
+ .rela.plt : { *(.rela.plt) *(.rela.iplt) }
+ . = 0x00020000;
+ .plt : { *(.plt) *(.iplt) }
+ . = 0x00030000;
+ .text : { *(.text) }
+ . = 0x20000000;
+ .got : { *(.got) *(.got.plt) }
+}
diff --git a/ld/testsuite/ld-aarch64/bti-far-3a.s b/ld/testsuite/ld-aarch64/bti-far-3a.s
new file mode 100644
index 00000000000..ed53545fed7
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3a.s
@@ -0,0 +1,23 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+ .global a_func
+ .type a_func, %function
+a_func:
+ b b_func
+ b extern_func
+
+.zero 0x07000000
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
diff --git a/ld/testsuite/ld-aarch64/bti-far-3b.s b/ld/testsuite/ld-aarch64/bti-far-3b.s
new file mode 100644
index 00000000000..1f3be3d15cf
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3b.s
@@ -0,0 +1,25 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+.zero 0x01000000
+
+ .global b_func
+ .type b_func, %function
+b_func:
+ b c_func
+ b a_func
+
+.zero 0x07000000
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
diff --git a/ld/testsuite/ld-aarch64/bti-far-3c.s b/ld/testsuite/ld-aarch64/bti-far-3c.s
new file mode 100644
index 00000000000..8e2361fd771
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/bti-far-3c.s
@@ -0,0 +1,24 @@
+ .text
+ .hidden a_func
+ .hidden b_func
+ .hidden c_func
+
+.zero 0x01000000
+
+ .global c_func
+ .type c_func, %function
+c_func:
+ b a_func
+ b b_func
+ b extern_func
+
+ .section .note.gnu.property,"a"
+ .align 3
+ .word 4
+ .word 16
+ .word 5
+ .string "GNU"
+ .word 3221225472
+ .word 4
+ .word 1
+ .align 3
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] aarch64 BTI stub fixes
2023-11-03 13:15 [PATCH 0/5] aarch64 BTI stub fixes Szabolcs Nagy
` (4 preceding siblings ...)
2023-11-03 13:22 ` [PATCH 5/5] ld: aarch64: Add BTI stub insertion test PR30930 Szabolcs Nagy
@ 2023-11-07 11:38 ` Nick Clifton
2023-11-07 13:08 ` Szabolcs Nagy
5 siblings, 1 reply; 12+ messages in thread
From: Nick Clifton @ 2023-11-07 11:38 UTC (permalink / raw)
To: Szabolcs Nagy, binutils
Hi Szabolcs,
> Large binaries with BTI can be linked incorrectly with
> binutils 2.41 because PR30930.
>
> there are other less critical related issues fixed in
> this patchset as well.
>
> Szabolcs Nagy (5):
> bfd: aarch64: Fix BTI stub optimization PR30957
> bfd: aarch64: Fix broken BTI stub PR30930
> bfd: aarch64: Fix leaks in case of BTI stub reuse
> bfd: aarch64: Avoid BTI stub for a PLT that has BTI
> ld: aarch64: Add BTI stub insertion test PR30930
This patch series looks good to me, so please apply if you have not already done so.
FYI - I did find one regression in the linker testsuite with the patch applied:
FAIL: Check linker stubs with indirect calls handle BTI (exe).
However there are mitigating circumstances:
* The failure was for an unusual toolchain configuration:
--target=aarch64_be-linux-gnu_ilp32
* The three other tests in this part of the testsuite also fail, both
with and without your patch applied:
FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
FAIL: Check linker stubs with indirect calls handle BTI (exe).
FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
I leave it up to you to decide if it is worth fixing these tests.
Cheers
Nick
PS. My gut feeling at the moment is that we do not need a 2.41.1 release
for this patch, since AArch64 systems with BTI enabled are not yet the norm,
and those that do exist are being used to test for problems like this rather
than general use. Since your patch will be in the 2.42 release, due January
next year, I think that that will be a reasonable timescale for getting the
fix out there. Do you agree ?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/5] aarch64 BTI stub fixes
2023-11-07 11:38 ` [PATCH 0/5] aarch64 BTI stub fixes Nick Clifton
@ 2023-11-07 13:08 ` Szabolcs Nagy
2023-11-09 14:58 ` Szabolcs Nagy
0 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-07 13:08 UTC (permalink / raw)
To: Nick Clifton, binutils
The 11/07/2023 11:38, Nick Clifton wrote:
> Hi Szabolcs,
>
> > Large binaries with BTI can be linked incorrectly with
> > binutils 2.41 because PR30930.
> >
> > there are other less critical related issues fixed in
> > this patchset as well.
> >
> > Szabolcs Nagy (5):
> > bfd: aarch64: Fix BTI stub optimization PR30957
> > bfd: aarch64: Fix broken BTI stub PR30930
> > bfd: aarch64: Fix leaks in case of BTI stub reuse
> > bfd: aarch64: Avoid BTI stub for a PLT that has BTI
> > ld: aarch64: Add BTI stub insertion test PR30930
>
> This patch series looks good to me, so please apply if you have not already done so.
>
>
> FYI - I did find one regression in the linker testsuite with the patch applied:
>
> FAIL: Check linker stubs with indirect calls handle BTI (exe).
>
> However there are mitigating circumstances:
>
> * The failure was for an unusual toolchain configuration:
>
> --target=aarch64_be-linux-gnu_ilp32
>
> * The three other tests in this part of the testsuite also fail, both
> with and without your patch applied:
>
> FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
> FAIL: Check linker stubs with indirect calls handle BTI (exe).
> FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
>
> Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
>
> I leave it up to you to decide if it is worth fixing these tests.
thanks for looking at this.
i will have to check what's going on.
> PS. My gut feeling at the moment is that we do not need a 2.41.1 release
> for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> and those that do exist are being used to test for problems like this rather
> than general use. Since your patch will be in the 2.42 release, due January
> next year, I think that that will be a reasonable timescale for getting the
> fix out there. Do you agree ?
makes sense,
unless it affects hardened kernel builds.
i will have to check if that's an issue.
thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] aarch64 BTI stub fixes
2023-11-07 13:08 ` Szabolcs Nagy
@ 2023-11-09 14:58 ` Szabolcs Nagy
2023-11-17 8:42 ` Fangrui Song
[not found] ` <DS7PR12MB5765CE9DC452CD430E68B89BCBB7A@DS7PR12MB5765.namprd12.prod.outlook.com>
0 siblings, 2 replies; 12+ messages in thread
From: Szabolcs Nagy @ 2023-11-09 14:58 UTC (permalink / raw)
To: Nick Clifton, binutils
The 11/07/2023 13:08, Szabolcs Nagy wrote:
> The 11/07/2023 11:38, Nick Clifton wrote:
> > FYI - I did find one regression in the linker testsuite with the patch applied:
> >
> > FAIL: Check linker stubs with indirect calls handle BTI (exe).
> >
> > However there are mitigating circumstances:
> >
> > * The failure was for an unusual toolchain configuration:
> >
> > --target=aarch64_be-linux-gnu_ilp32
> >
> > * The three other tests in this part of the testsuite also fail, both
> > with and without your patch applied:
> >
> > FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
> > FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
> >
> > Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
> >
> > I leave it up to you to decide if it is worth fixing these tests.
>
> thanks for looking at this.
> i will have to check what's going on.
i will change the name of the new test to be unique.
cleaning up ilp32 test issues will be a fair bit of work.
i can commit a followup patch to force the tests i added to use
lp64 abi, like we do on other tests, but this is not ideal.
> > PS. My gut feeling at the moment is that we do not need a 2.41.1 release
> > for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> > and those that do exist are being used to test for problems like this rather
> > than general use. Since your patch will be in the 2.42 release, due January
> > next year, I think that that will be a reasonable timescale for getting the
> > fix out there. Do you agree ?
>
> makes sense,
> unless it affects hardened kernel builds.
> i will have to check if that's an issue.
in practice i think the kernel should be fine.
(it's hard to say conclusively)
but i plan to backport the patches even if we don't arrange
a new release.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/5] aarch64 BTI stub fixes
2023-11-09 14:58 ` Szabolcs Nagy
@ 2023-11-17 8:42 ` Fangrui Song
[not found] ` <DS7PR12MB5765CE9DC452CD430E68B89BCBB7A@DS7PR12MB5765.namprd12.prod.outlook.com>
1 sibling, 0 replies; 12+ messages in thread
From: Fangrui Song @ 2023-11-17 8:42 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: Nick Clifton, binutils
On Thu, Nov 9, 2023 at 6:58 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 11/07/2023 13:08, Szabolcs Nagy wrote:
> > The 11/07/2023 11:38, Nick Clifton wrote:
> > > FYI - I did find one regression in the linker testsuite with the patch applied:
> > >
> > > FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > >
> > > However there are mitigating circumstances:
> > >
> > > * The failure was for an unusual toolchain configuration:
> > >
> > > --target=aarch64_be-linux-gnu_ilp32
> > >
> > > * The three other tests in this part of the testsuite also fail, both
> > > with and without your patch applied:
> > >
> > > FAIL: Check linker stubs with indirect calls handle BTI (shared lib).
> > > FAIL: Check linker stubs with indirect calls handle BTI (exe).
> > > FAIL: Check linker stubs with indirect calls handle BTI when target has BTI.
> > >
> > > Note: there appear to be *two* tests called "check linker stubs with indirect calls handle BTI (exe)."
> > >
> > > I leave it up to you to decide if it is worth fixing these tests.
> >
> > thanks for looking at this.
> > i will have to check what's going on.
>
> i will change the name of the new test to be unique.
>
> cleaning up ilp32 test issues will be a fair bit of work.
>
> i can commit a followup patch to force the tests i added to use
> lp64 abi, like we do on other tests, but this is not ideal.
>
> > > PS. My gut feeling at the moment is that we do not need a 2.41.1 release
> > > for this patch, since AArch64 systems with BTI enabled are not yet the norm,
> > > and those that do exist are being used to test for problems like this rather
> > > than general use. Since your patch will be in the 2.42 release, due January
> > > next year, I think that that will be a reasonable timescale for getting the
> > > fix out there. Do you agree ?
> >
> > makes sense,
> > unless it affects hardened kernel builds.
> > i will have to check if that's an issue.
>
> in practice i think the kernel should be fine.
> (it's hard to say conclusively)
>
> but i plan to backport the patches even if we don't arrange
> a new release.
% grep '\.zero' ld/testsuite/ld-aarch64/bti-far*
ld/testsuite/ld-aarch64/bti-far-3a.s:.zero 0x07000000
ld/testsuite/ld-aarch64/bti-far-3b.s:.zero 0x01000000
ld/testsuite/ld-aarch64/bti-far-3b.s:.zero 0x07000000
ld/testsuite/ld-aarch64/bti-far-3c.s:.zero 0x01000000
% size ld/tmpdir/bti-far-*
text data bss dec hex filename
117440552 0 0 117440552 7000028 ld/tmpdir/bti-far-3a.o
134217768 0 0 134217768 8000028 ld/tmpdir/bti-far-3b.o
16777260 0 0 16777260 100002c ld/tmpdir/bti-far-3c.o
132 0 0 132 84 ld/tmpdir/bti-far-opt.o
`.zero 0x07000000` is 112MiB. Perhaps use output section
addresses (e.g. [1]) to make the files smaller?
[1]: https://github.com/llvm/llvm-project/blob/de176d8c5496d6cf20e82aface98e102c593dbe2/lld/test/ELF/aarch64-thunk-pi.s#L112
^ permalink raw reply [flat|nested] 12+ messages in thread[parent not found: <DS7PR12MB5765CE9DC452CD430E68B89BCBB7A@DS7PR12MB5765.namprd12.prod.outlook.com>]