This series implements support for emitting the data structures associated
with jump tables as 32-bit relative references instead of absolute
references, which take up more space on builds that target 64-bit
architectures, or implement self relocation [or both].
This series enables it for arm64 and x86, although other architectures
might benefit as well.
Patch #1 does some preparatory refactoring before patch #2 introduces the
generic pieces required for using relative references.
Patch #3 wires everything up for arm64.
For x86, patch #4 applies some preparatory changes for the arch specific
jump label C code, which is a lot more involved than on arm64, which is
why it is split off in this case. Patch #5 wires it up for x86 as well.
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Ard Biesheuvel (5):
kernel/jump_label: abstract jump_entry member accessors
kernel/jump_label: implement generic support for relative references
arm64/kernel: jump_label: switch to relative references
x86: jump_label: switch to jump_entry accessors
x86/kernel: jump_table: use relative references
arch/Kconfig | 3 +
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/jump_label.h | 16 ++---
arch/arm64/kernel/jump_label.c | 6 +-
arch/x86/Kconfig | 1 +
arch/x86/include/asm/jump_label.h | 28 +++------
arch/x86/kernel/jump_label.c | 43 +++++++------
include/linux/jump_label.h | 64 ++++++++++++++++++++
kernel/jump_label.c | 56 ++++++++++-------
tools/objtool/special.c | 4 +-
10 files changed, 145 insertions(+), 77 deletions(-)
--
2.11.0
On a randomly chosen distro kernel build for arm64, vmlinux.o shows the
following sections, containing jump label entries, and the associated
RELA relocation records, respectively:
...
[38088] __jump_table PROGBITS 0000000000000000 00e19f30
000000000002ea10 0000000000000000 WA 0 0 8
[38089] .rela__jump_table RELA 0000000000000000 01fd8bb0
000000000008be30 0000000000000018 I 38178 38088 8
...
In other words, we have 190 KB worth of 'struct jump_entry' instances,
and 573 KB worth of RELA entries to relocate each entry's code, target
and key members. This means the RELA section occupies 10% of the .init
segment, and the two sections combined represent 5% of vmlinux's entire
memory footprint.
So let's switch from 64-bit absolute references to 32-bit relative
references: this reduces the size of the __jump_table by 50%, and gets
rid of the RELA section entirely.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/jump_label.h | 16 ++++------------
arch/arm64/kernel/jump_label.c | 6 +++---
3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1940c6405d04..d17aa9614e69 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -91,6 +91,7 @@ config ARM64
select HAVE_ARCH_BITREVERSE
select HAVE_ARCH_HUGE_VMAP
select HAVE_ARCH_JUMP_LABEL
+ select HAVE_ARCH_JUMP_LABEL_RELATIVE
select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
select HAVE_ARCH_KGDB
select HAVE_ARCH_MMAP_RND_BITS
diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
index 1b5e0e843c3a..5e3e31df3805 100644
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -30,8 +30,8 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
{
asm goto("1: nop\n\t"
".pushsection __jump_table, \"aw\"\n\t"
- ".align 3\n\t"
- ".quad 1b, %l[l_yes], %c0\n\t"
+ ".align 2\n\t"
+ ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
".popsection\n\t"
: : "i"(&((char *)key)[branch]) : : l_yes);
@@ -44,8 +44,8 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
{
asm goto("1: b %l[l_yes]\n\t"
".pushsection __jump_table, \"aw\"\n\t"
- ".align 3\n\t"
- ".quad 1b, %l[l_yes], %c0\n\t"
+ ".align 2\n\t"
+ ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
".popsection\n\t"
: : "i"(&((char *)key)[branch]) : : l_yes);
@@ -54,13 +54,5 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
return true;
}
-typedef u64 jump_label_t;
-
-struct jump_entry {
- jump_label_t code;
- jump_label_t target;
- jump_label_t key;
-};
-
#endif /* __ASSEMBLY__ */
#endif /* __ASM_JUMP_LABEL_H */
diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c
index c2dd1ad3e648..903d17023d77 100644
--- a/arch/arm64/kernel/jump_label.c
+++ b/arch/arm64/kernel/jump_label.c
@@ -25,12 +25,12 @@
void arch_jump_label_transform(struct jump_entry *entry,
enum jump_label_type type)
{
- void *addr = (void *)entry->code;
+ void *addr = (void *)jump_entry_code(entry);
u32 insn;
if (type == JUMP_LABEL_JMP) {
- insn = aarch64_insn_gen_branch_imm(entry->code,
- entry->target,
+ insn = aarch64_insn_gen_branch_imm(jump_entry_code(entry),
+ jump_entry_target(entry),
AARCH64_INSN_BRANCH_NOLINK);
} else {
insn = aarch64_insn_gen_nop();
--
2.11.0
In preparation of switching x86 to use place-relative references for
the code, target and key members of struct jump_entry, replace direct
references to the struct members with invocations of the new accessors.
This will allow us to make the switch by modifying the accessors only.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/kernel/jump_label.c | 43 ++++++++++++--------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index e56c95be2808..d64296092ef5 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -52,22 +52,24 @@ static void __jump_label_transform(struct jump_entry *entry,
* Jump label is enabled for the first time.
* So we expect a default_nop...
*/
- if (unlikely(memcmp((void *)entry->code, default_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ default_nop, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
} else {
/*
* ...otherwise expect an ideal_nop. Otherwise
* something went horribly wrong.
*/
- if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ ideal_nop, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
}
code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
+ code.offset = jump_entry_target(entry) -
+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
} else {
/*
* We are disabling this jump label. If it is not what
@@ -76,14 +78,18 @@ static void __jump_label_transform(struct jump_entry *entry,
* are converting the default nop to the ideal nop.
*/
if (init) {
- if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
- bug_at((void *)entry->code, __LINE__);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ default_nop, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
} else {
code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
- if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))
- bug_at((void *)entry->code, __LINE__);
+ code.offset = jump_entry_target(entry) -
+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ &code, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
}
memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
}
@@ -97,10 +103,13 @@ static void __jump_label_transform(struct jump_entry *entry,
*
*/
if (poker)
- (*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
+ (*poker)((void *)jump_entry_code(entry), &code,
+ JUMP_LABEL_NOP_SIZE);
else
- text_poke_bp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE,
- (void *)entry->code + JUMP_LABEL_NOP_SIZE);
+ text_poke_bp((void *)jump_entry_code(entry), &code,
+ JUMP_LABEL_NOP_SIZE,
+ (void *)jump_entry_code(entry) +
+ JUMP_LABEL_NOP_SIZE);
}
void arch_jump_label_transform(struct jump_entry *entry,
--
2.11.0
In preparation of allowing architectures to use relative references
in jump_label entries [which can dramatically reduce the memory
footprint], introduce abstractions for references to the 'code' and
'key' members of struct jump_entry.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
include/linux/jump_label.h | 36 ++++++++++++++++++++
kernel/jump_label.c | 36 ++++++++------------
2 files changed, 50 insertions(+), 22 deletions(-)
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index b46b541c67c4..86ec0652d3b1 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -119,6 +119,42 @@ struct static_key {
#ifdef HAVE_JUMP_LABEL
#include <asm/jump_label.h>
+
+#ifndef __ASSEMBLY__
+
+struct jump_entry; /* defined by the architecture */
+
+static inline unsigned long jump_entry_code(const struct jump_entry *entry)
+{
+ return entry->code;
+}
+
+static inline unsigned long jump_entry_target(const struct jump_entry *entry)
+{
+ return entry->target;
+}
+
+static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
+{
+ return (struct static_key *)((unsigned long)entry->key & ~1UL);
+}
+
+static inline bool jump_entry_is_branch(const struct jump_entry *entry)
+{
+ return (unsigned long)entry->key & 1UL;
+}
+
+static inline bool jump_entry_is_module_init(const struct jump_entry *entry)
+{
+ return entry->code == 0;
+}
+
+static inline void jump_entry_set_module_init(struct jump_entry *entry)
+{
+ entry->code = 0;
+}
+
+#endif
#endif
#ifndef __ASSEMBLY__
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 01ebdf1f9f40..c3524c9b3004 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -38,10 +38,12 @@ static int jump_label_cmp(const void *a, const void *b)
const struct jump_entry *jea = a;
const struct jump_entry *jeb = b;
- if (jea->key < jeb->key)
+ if ((unsigned long)jump_entry_key(jea) <
+ (unsigned long)jump_entry_key(jeb))
return -1;
- if (jea->key > jeb->key)
+ if ((unsigned long)jump_entry_key(jea) >
+ (unsigned long)jump_entry_key(jeb))
return 1;
return 0;
@@ -261,8 +263,8 @@ EXPORT_SYMBOL_GPL(jump_label_rate_limit);
static int addr_conflict(struct jump_entry *entry, void *start, void *end)
{
- if (entry->code <= (unsigned long)end &&
- entry->code + JUMP_LABEL_NOP_SIZE > (unsigned long)start)
+ if (jump_entry_code(entry) <= (unsigned long)end &&
+ jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE > (unsigned long)start)
return 1;
return 0;
@@ -321,16 +323,6 @@ static inline void static_key_set_linked(struct static_key *key)
key->type |= JUMP_TYPE_LINKED;
}
-static inline struct static_key *jump_entry_key(struct jump_entry *entry)
-{
- return (struct static_key *)((unsigned long)entry->key & ~1UL);
-}
-
-static bool jump_entry_branch(struct jump_entry *entry)
-{
- return (unsigned long)entry->key & 1UL;
-}
-
/***
* A 'struct static_key' uses a union such that it either points directly
* to a table of 'struct jump_entry' or to a linked list of modules which in
@@ -355,7 +347,7 @@ static enum jump_label_type jump_label_type(struct jump_entry *entry)
{
struct static_key *key = jump_entry_key(entry);
bool enabled = static_key_enabled(key);
- bool branch = jump_entry_branch(entry);
+ bool branch = jump_entry_is_branch(entry);
/* See the comment in linux/jump_label.h */
return enabled ^ branch;
@@ -370,8 +362,8 @@ static void __jump_label_update(struct static_key *key,
* An entry->code of 0 indicates an entry which has been
* disabled because it was in an init text area.
*/
- if (entry->code) {
- if (kernel_text_address(entry->code))
+ if (!jump_entry_is_module_init(entry)) {
+ if (kernel_text_address(jump_entry_code(entry)))
arch_jump_label_transform(entry, jump_label_type(entry));
else
WARN_ONCE(1, "can't patch jump_label at %pS",
@@ -441,7 +433,7 @@ static enum jump_label_type jump_label_init_type(struct jump_entry *entry)
{
struct static_key *key = jump_entry_key(entry);
bool type = static_key_type(key);
- bool branch = jump_entry_branch(entry);
+ bool branch = jump_entry_is_branch(entry);
/* See the comment in linux/jump_label.h */
return type ^ branch;
@@ -565,7 +557,7 @@ static int jump_label_add_module(struct module *mod)
continue;
key = iterk;
- if (within_module(iter->key, mod)) {
+ if (within_module((unsigned long)key, mod)) {
static_key_set_entries(key, iter);
continue;
}
@@ -615,7 +607,7 @@ static void jump_label_del_module(struct module *mod)
key = jump_entry_key(iter);
- if (within_module(iter->key, mod))
+ if (within_module((unsigned long)key, mod))
continue;
/* No memory during module load */
@@ -659,8 +651,8 @@ static void jump_label_invalidate_module_init(struct module *mod)
struct jump_entry *iter;
for (iter = iter_start; iter < iter_stop; iter++) {
- if (within_module_init(iter->code, mod))
- iter->code = 0;
+ if (within_module_init(jump_entry_code(iter), mod))
+ jump_entry_set_module_init(iter);
}
}
--
2.11.0
Similar to the arm64 case, 64-bit x86 can benefit from using 32-bit
relative references rather than 64-bit absolute ones when emitting
struct jump_entry instances. Not only does this reduce the memory
footprint of the entries themselves by 50%, it also removes the need
for carrying relocation metadata on relocatable builds (i.e., for KASLR)
which saves a fair chunk of .init space as well (although the savings
are not as dramatic as on arm64)
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/include/asm/jump_label.h | 28 ++++++--------------
tools/objtool/special.c | 4 +--
3 files changed, 11 insertions(+), 22 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e10a3542db7e..dd71258ec1cc 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -118,6 +118,7 @@ config X86
select HAVE_ARCH_AUDITSYSCALL
select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
select HAVE_ARCH_JUMP_LABEL
+ select HAVE_ARCH_JUMP_LABEL_RELATIVE
select HAVE_ARCH_KASAN if X86_64
select HAVE_ARCH_KGDB
select HAVE_ARCH_MMAP_RND_BITS if MMU
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 8c0de4282659..d0f1f25b41d5 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -36,8 +36,8 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
asm_volatile_goto("1:"
".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
".pushsection __jump_table, \"aw\" \n\t"
- _ASM_ALIGN "\n\t"
- _ASM_PTR "1b, %l[l_yes], %c0 + %c1 \n\t"
+ ".balign 4\n\t"
+ ".long 1b - ., %l[l_yes] - ., %c0 + %c1 - .\n\t"
".popsection \n\t"
: : "i" (key), "i" (branch) : : l_yes);
@@ -52,8 +52,8 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
".byte 0xe9\n\t .long %l[l_yes] - 2f\n\t"
"2:\n\t"
".pushsection __jump_table, \"aw\" \n\t"
- _ASM_ALIGN "\n\t"
- _ASM_PTR "1b, %l[l_yes], %c0 + %c1 \n\t"
+ ".balign 4\n\t"
+ ".long 1b - ., %l[l_yes] - ., %c0 + %c1 - .\n\t"
".popsection \n\t"
: : "i" (key), "i" (branch) : : l_yes);
@@ -62,18 +62,6 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
return true;
}
-#ifdef CONFIG_X86_64
-typedef u64 jump_label_t;
-#else
-typedef u32 jump_label_t;
-#endif
-
-struct jump_entry {
- jump_label_t code;
- jump_label_t target;
- jump_label_t key;
-};
-
#else /* __ASSEMBLY__ */
.macro STATIC_JUMP_IF_TRUE target, key, def
@@ -87,8 +75,8 @@ struct jump_entry {
.byte STATIC_KEY_INIT_NOP
.endif
.pushsection __jump_table, "aw"
- _ASM_ALIGN
- _ASM_PTR .Lstatic_jump_\@, \target, \key
+ .balign 4
+ .long .Lstatic_jump_\@ - ., \target - ., \key - .
.popsection
.endm
@@ -103,8 +91,8 @@ struct jump_entry {
.Lstatic_jump_after_\@:
.endif
.pushsection __jump_table, "aw"
- _ASM_ALIGN
- _ASM_PTR .Lstatic_jump_\@, \target, \key + 1
+ .balign 4
+ .long .Lstatic_jump_\@ - ., \target - ., \key + 1 - .
.popsection
.endm
diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index 84f001d52322..98ae55b39037 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -30,9 +30,9 @@
#define EX_ORIG_OFFSET 0
#define EX_NEW_OFFSET 4
-#define JUMP_ENTRY_SIZE 24
+#define JUMP_ENTRY_SIZE 12
#define JUMP_ORIG_OFFSET 0
-#define JUMP_NEW_OFFSET 8
+#define JUMP_NEW_OFFSET 4
#define ALT_ENTRY_SIZE 13
#define ALT_ORIG_OFFSET 0
--
2.11.0
To reduce the size taken up by absolute references in jump label
entries themselves and the associated relocation records in the
.init segment, add support for emitting them as 32-bit relative
references instead.
Note that this requires some extra care in the sorting routine, given
that the offsets change when entries are moved around in the jump_entry
table.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/Kconfig | 3 +++
include/linux/jump_label.h | 28 ++++++++++++++++++++
kernel/jump_label.c | 20 +++++++++++++-
3 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 2b8b70820002..22fa3792626e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -348,6 +348,9 @@ config HAVE_PERF_USER_STACK_DUMP
config HAVE_ARCH_JUMP_LABEL
bool
+config HAVE_ARCH_JUMP_LABEL_RELATIVE
+ bool
+
config HAVE_RCU_TABLE_FREE
bool
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 86ec0652d3b1..aa203dffe72c 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -121,6 +121,32 @@ struct static_key {
#include <asm/jump_label.h>
#ifndef __ASSEMBLY__
+#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
+
+struct jump_entry {
+ int code;
+ int target;
+ int key;
+};
+
+static inline unsigned long jump_entry_code(const struct jump_entry *entry)
+{
+ return (unsigned long)&entry->code + entry->code;
+}
+
+static inline unsigned long jump_entry_target(const struct jump_entry *entry)
+{
+ return (unsigned long)&entry->target + entry->target;
+}
+
+static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
+{
+ unsigned long key = (unsigned long)&entry->key + entry->key;
+
+ return (struct static_key *)(key & ~1UL);
+}
+
+#else
struct jump_entry; /* defined by the architecture */
@@ -139,6 +165,8 @@ static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
return (struct static_key *)((unsigned long)entry->key & ~1UL);
}
+#endif
+
static inline bool jump_entry_is_branch(const struct jump_entry *entry)
{
return (unsigned long)entry->key & 1UL;
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index c3524c9b3004..285eff13ecd1 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -49,6 +49,22 @@ static int jump_label_cmp(const void *a, const void *b)
return 0;
}
+static void jump_label_swap(void *a, void *b, int size)
+{
+ long delta = (unsigned long)a - (unsigned long)b;
+ struct jump_entry *jea = a;
+ struct jump_entry *jeb = b;
+ struct jump_entry tmp = *jea;
+
+ jea->code = jeb->code - delta;
+ jea->target = jeb->target - delta;
+ jea->key = jeb->key - delta;
+
+ jeb->code = tmp.code + delta;
+ jeb->target = tmp.target + delta;
+ jeb->key = tmp.key + delta;
+}
+
static void
jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
{
@@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
size = (((unsigned long)stop - (unsigned long)start)
/ sizeof(struct jump_entry));
- sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
+ sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
+ IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
+ : NULL);
}
static void jump_label_update(struct static_key *key);
--
2.11.0
On Wed, Jun 27, 2018 at 06:06:04PM +0200, Ard Biesheuvel wrote:
> Similar to the arm64 case, 64-bit x86 can benefit from using 32-bit
> relative references rather than 64-bit absolute ones when emitting
> struct jump_entry instances. Not only does this reduce the memory
> footprint of the entries themselves by 50%, it also removes the need
> for carrying relocation metadata on relocatable builds (i.e., for KASLR)
> which saves a fair chunk of .init space as well (although the savings
> are not as dramatic as on arm64)
This will conflict with:
https://lkml.kernel.org/r/[email protected]
On 28 June 2018 at 10:31, Peter Zijlstra <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 06:06:04PM +0200, Ard Biesheuvel wrote:
>> Similar to the arm64 case, 64-bit x86 can benefit from using 32-bit
>> relative references rather than 64-bit absolute ones when emitting
>> struct jump_entry instances. Not only does this reduce the memory
>> footprint of the entries themselves by 50%, it also removes the need
>> for carrying relocation metadata on relocatable builds (i.e., for KASLR)
>> which saves a fair chunk of .init space as well (although the savings
>> are not as dramatic as on arm64)
>
> This will conflict with:
>
> https://lkml.kernel.org/r/[email protected]
Thanks for the head's up. Fortunately, it does not conflict
fundamentally, so it should be a straight-forward rebase after that
code is merged.
Do you think this is likely to get merged for v4.19?
On Wed, Jun 27, 2018 at 06:06:00PM +0200, Ard Biesheuvel wrote:
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 01ebdf1f9f40..c3524c9b3004 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -38,10 +38,12 @@ static int jump_label_cmp(const void *a, const void *b)
> const struct jump_entry *jea = a;
> const struct jump_entry *jeb = b;
>
> - if (jea->key < jeb->key)
> + if ((unsigned long)jump_entry_key(jea) <
> + (unsigned long)jump_entry_key(jeb))
> return -1;
>
> - if (jea->key > jeb->key)
> + if ((unsigned long)jump_entry_key(jea) >
> + (unsigned long)jump_entry_key(jeb))
> return 1;
I think you can ditch the unsigned long cast and directly compare
pointers. That leads to much prettier code:
if (jump_entry_key(jea) < jump_entry_key(jeb))
return -1;
etc..
On 28 June 2018 at 10:50, Peter Zijlstra <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote:
>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>> index 86ec0652d3b1..aa203dffe72c 100644
>> --- a/include/linux/jump_label.h
>> +++ b/include/linux/jump_label.h
>> @@ -121,6 +121,32 @@ struct static_key {
>> #include <asm/jump_label.h>
>>
>> #ifndef __ASSEMBLY__
>> +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
>> +
>> +struct jump_entry {
>> + int code;
>> + int target;
>> + int key;
>> +};
>
> I much prefer you use 'u32' there.
>
Actually, they are signed so that would be s32. But yeah, I can change that.
>
>> +static void jump_label_swap(void *a, void *b, int size)
>> +{
>> + long delta = (unsigned long)a - (unsigned long)b;
>> + struct jump_entry *jea = a;
>> + struct jump_entry *jeb = b;
>> + struct jump_entry tmp = *jea;
>> +
>> + jea->code = jeb->code - delta;
>> + jea->target = jeb->target - delta;
>> + jea->key = jeb->key - delta;
>> +
>> + jeb->code = tmp.code + delta;
>> + jeb->target = tmp.target + delta;
>> + jeb->key = tmp.key + delta;
>> +}
>> +
>> static void
>> jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>> {
>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>>
>> size = (((unsigned long)stop - (unsigned long)start)
>> / sizeof(struct jump_entry));
>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
>> + : NULL);
>> }
>
> That will result in jump_label_swap being an unused symbol for some
> compile options.
>
No, and isn't that the point of IS_ENABLED()? The compiler sees a
reference to jump_label_swap(), so it won't complain about it being
unused.
> Would it not be much nicer to write that like:
>
> static void jump_label_swap(void *a, void *b, int size)
> {
> struct jump_entry *jea = a, *jeb = b;
>
> #ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
> long delta = a - b;
>
> jea->code += delta;
> jea->target += delta;
> jea->key += delta;
>
> jeb->code -= delta;
> jeb->target -= delta;
> jeb->key -= delta;
> #else
>
> swap(*jea, *jeb);
> }
>
> And then unconditionally use jump_label_swap().
Meh. I thought IS_ENABLED() was preferred over #ifdef, no? That way,
the compiler always sees the code, and simply discards it without
complaining if it ends up left unused.
On 28 June 2018 at 11:02, Ard Biesheuvel <[email protected]> wrote:
> On 28 June 2018 at 10:50, Peter Zijlstra <[email protected]> wrote:
>> On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote:
>>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>>> index 86ec0652d3b1..aa203dffe72c 100644
>>> --- a/include/linux/jump_label.h
>>> +++ b/include/linux/jump_label.h
>>> @@ -121,6 +121,32 @@ struct static_key {
>>> #include <asm/jump_label.h>
>>>
>>> #ifndef __ASSEMBLY__
>>> +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
>>> +
>>> +struct jump_entry {
>>> + int code;
>>> + int target;
>>> + int key;
>>> +};
>>
>> I much prefer you use 'u32' there.
>>
>
> Actually, they are signed so that would be s32. But yeah, I can change that.
>
>>
>>> +static void jump_label_swap(void *a, void *b, int size)
>>> +{
>>> + long delta = (unsigned long)a - (unsigned long)b;
>>> + struct jump_entry *jea = a;
>>> + struct jump_entry *jeb = b;
>>> + struct jump_entry tmp = *jea;
>>> +
>>> + jea->code = jeb->code - delta;
>>> + jea->target = jeb->target - delta;
>>> + jea->key = jeb->key - delta;
>>> +
>>> + jeb->code = tmp.code + delta;
>>> + jeb->target = tmp.target + delta;
>>> + jeb->key = tmp.key + delta;
>>> +}
>>> +
>>> static void
>>> jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>>> {
>>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>>>
>>> size = (((unsigned long)stop - (unsigned long)start)
>>> / sizeof(struct jump_entry));
>>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
>>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
>>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
>>> + : NULL);
>>> }
>>
>> That will result in jump_label_swap being an unused symbol for some
>> compile options.
>>
>
> No, and isn't that the point of IS_ENABLED()? The compiler sees a
> reference to jump_label_swap(), so it won't complain about it being
> unused.
>
>> Would it not be much nicer to write that like:
>>
>> static void jump_label_swap(void *a, void *b, int size)
>> {
>> struct jump_entry *jea = a, *jeb = b;
>>
>> #ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
>> long delta = a - b;
>>
>> jea->code += delta;
>> jea->target += delta;
>> jea->key += delta;
>>
>> jeb->code -= delta;
>> jeb->target -= delta;
>> jeb->key -= delta;
>> #else
>>
>> swap(*jea, *jeb);
>> }
>>
>> And then unconditionally use jump_label_swap().
>
> Meh. I thought IS_ENABLED() was preferred over #ifdef, no? That way,
> the compiler always sees the code, and simply discards it without
> complaining if it ends up left unused.
... and it means the sort() routine will unconditionally perform an
indirect function call even if the arch does not require it.
On Wed, Jun 27, 2018 at 06:06:03PM +0200, Ard Biesheuvel wrote:
> In preparation of switching x86 to use place-relative references for
> the code, target and key members of struct jump_entry, replace direct
> references to the struct members with invocations of the new accessors.
> This will allow us to make the switch by modifying the accessors only.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
That just makes horrible code worse.. can't we do something like so
instead?
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -42,52 +42,37 @@ static void __jump_label_transform(struc
void *(*poker)(void *, const void *, size_t),
int init)
{
- union jump_code_union code;
+ union jump_code_union jmp = {
+ { .jump = 0xe9,
+ .offset = jump_entry_target(entry) -
+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE), }
+ };
const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
+ const unsigned char *expect, *code;
+ int line;
if (type == JUMP_LABEL_JMP) {
if (init) {
- /*
- * Jump label is enabled for the first time.
- * So we expect a default_nop...
- */
- if (unlikely(memcmp((void *)entry->code, default_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ expect = default_nop; line = __LINE__;
} else {
- /*
- * ...otherwise expect an ideal_nop. Otherwise
- * something went horribly wrong.
- */
- if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ expect = ideal_nop; line = __LINE__;
}
- code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
+ code = &jmp.code;
} else {
- /*
- * We are disabling this jump label. If it is not what
- * we think it is, then something must have gone wrong.
- * If this is the first initialization call, then we
- * are converting the default nop to the ideal nop.
- */
if (init) {
- if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
- bug_at((void *)entry->code, __LINE__);
+ expect = default_nop; line = __LINE__;
} else {
- code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
- if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))
- bug_at((void *)entry->code, __LINE__);
+ expect = &jmp.code; line = __LINE__;
}
- memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
+
+ code = ideal_nop;
}
+ if (memcmp((void *)jump_entry_code(entry), expect, JUMP_LABEL_NOP_SIZE))
+ bug_at((void *)jump_entry_code(entry), line);
+
/*
* Make text_poke_bp() a default fallback poker.
*
@@ -96,11 +81,13 @@ static void __jump_label_transform(struc
* always nop being the 'currently valid' instruction
*
*/
- if (poker)
- (*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
- else
- text_poke_bp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE,
- (void *)entry->code + JUMP_LABEL_NOP_SIZE);
+ if (poker) {
+ (*poker)((void *)jump_entry_code(entry), &code, JUMP_LABEL_NOP_SIZE);
+ return;
+ }
+
+ text_poke_bp((void *)jump_entry_code(entry), &code, JUMP_LABEL_NOP_SIZE,
+ (void *)jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
}
void arch_jump_label_transform(struct jump_entry *entry,
On 28 June 2018 at 11:11, Peter Zijlstra <[email protected]> wrote:
> On Wed, Jun 27, 2018 at 06:06:03PM +0200, Ard Biesheuvel wrote:
>> In preparation of switching x86 to use place-relative references for
>> the code, target and key members of struct jump_entry, replace direct
>> references to the struct members with invocations of the new accessors.
>> This will allow us to make the switch by modifying the accessors only.
>>
>> Signed-off-by: Ard Biesheuvel <[email protected]>
>
> That just makes horrible code worse.. can't we do something like so
> instead?
>
Sure, I will incorporate that for v2.
>
> --- a/arch/x86/kernel/jump_label.c
> +++ b/arch/x86/kernel/jump_label.c
> @@ -42,52 +42,37 @@ static void __jump_label_transform(struc
> void *(*poker)(void *, const void *, size_t),
> int init)
> {
> - union jump_code_union code;
> + union jump_code_union jmp = {
> + { .jump = 0xe9,
> + .offset = jump_entry_target(entry) -
> + (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE), }
> + };
> const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
> const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
> + const unsigned char *expect, *code;
> + int line;
>
> if (type == JUMP_LABEL_JMP) {
> if (init) {
> - /*
> - * Jump label is enabled for the first time.
> - * So we expect a default_nop...
> - */
> - if (unlikely(memcmp((void *)entry->code, default_nop, 5)
> - != 0))
> - bug_at((void *)entry->code, __LINE__);
> + expect = default_nop; line = __LINE__;
> } else {
> - /*
> - * ...otherwise expect an ideal_nop. Otherwise
> - * something went horribly wrong.
> - */
> - if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
> - != 0))
> - bug_at((void *)entry->code, __LINE__);
> + expect = ideal_nop; line = __LINE__;
> }
>
> - code.jump = 0xe9;
> - code.offset = entry->target -
> - (entry->code + JUMP_LABEL_NOP_SIZE);
> + code = &jmp.code;
> } else {
> - /*
> - * We are disabling this jump label. If it is not what
> - * we think it is, then something must have gone wrong.
> - * If this is the first initialization call, then we
> - * are converting the default nop to the ideal nop.
> - */
> if (init) {
> - if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
> - bug_at((void *)entry->code, __LINE__);
> + expect = default_nop; line = __LINE__;
> } else {
> - code.jump = 0xe9;
> - code.offset = entry->target -
> - (entry->code + JUMP_LABEL_NOP_SIZE);
> - if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))
> - bug_at((void *)entry->code, __LINE__);
> + expect = &jmp.code; line = __LINE__;
> }
> - memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
> +
> + code = ideal_nop;
> }
>
> + if (memcmp((void *)jump_entry_code(entry), expect, JUMP_LABEL_NOP_SIZE))
> + bug_at((void *)jump_entry_code(entry), line);
> +
> /*
> * Make text_poke_bp() a default fallback poker.
> *
> @@ -96,11 +81,13 @@ static void __jump_label_transform(struc
> * always nop being the 'currently valid' instruction
> *
> */
> - if (poker)
> - (*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
> - else
> - text_poke_bp((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE,
> - (void *)entry->code + JUMP_LABEL_NOP_SIZE);
> + if (poker) {
> + (*poker)((void *)jump_entry_code(entry), &code, JUMP_LABEL_NOP_SIZE);
> + return;
> + }
> +
> + text_poke_bp((void *)jump_entry_code(entry), &code, JUMP_LABEL_NOP_SIZE,
> + (void *)jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
> }
>
> void arch_jump_label_transform(struct jump_entry *entry,
On Wed, Jun 27, 2018 at 06:06:02PM +0200, Ard Biesheuvel wrote:
> On a randomly chosen distro kernel build for arm64, vmlinux.o shows the
> following sections, containing jump label entries, and the associated
> RELA relocation records, respectively:
>
> ...
> [38088] __jump_table PROGBITS 0000000000000000 00e19f30
> 000000000002ea10 0000000000000000 WA 0 0 8
> [38089] .rela__jump_table RELA 0000000000000000 01fd8bb0
> 000000000008be30 0000000000000018 I 38178 38088 8
> ...
>
> In other words, we have 190 KB worth of 'struct jump_entry' instances,
> and 573 KB worth of RELA entries to relocate each entry's code, target
> and key members. This means the RELA section occupies 10% of the .init
> segment, and the two sections combined represent 5% of vmlinux's entire
> memory footprint.
>
> So let's switch from 64-bit absolute references to 32-bit relative
> references: this reduces the size of the __jump_table by 50%, and gets
> rid of the RELA section entirely.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/jump_label.h | 16 ++++------------
> arch/arm64/kernel/jump_label.c | 6 +++---
> 3 files changed, 8 insertions(+), 15 deletions(-)
Looks good, cheers:
Acked-by: Will Deacon <[email protected]>
Will
On Thu, Jun 28, 2018 at 11:04:45AM +0200, Ard Biesheuvel wrote:
> On 28 June 2018 at 11:02, Ard Biesheuvel <[email protected]> wrote:
> >>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
> >>>
> >>> size = (((unsigned long)stop - (unsigned long)start)
> >>> / sizeof(struct jump_entry));
> >>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
> >>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
> >>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
> >>> + : NULL);
> >>> }
> >>
> >> That will result in jump_label_swap being an unused symbol for some
> >> compile options.
> >
> > No, and isn't that the point of IS_ENABLED()? The compiler sees a
> > reference to jump_label_swap(), so it won't complain about it being
> > unused.
Ah, ok. I hadn't figured it was quite that smart about it.
> > Meh. I thought IS_ENABLED() was preferred over #ifdef, no?
Dunno, I just reacted to the proposed code's uglyness :-)
> ... and it means the sort() routine will unconditionally perform an
> indirect function call even if the arch does not require it.
Yeah, not sure I care about that here, this is a one time affair, very
far away from any fast paths.
On Thu, Jun 28, 2018 at 10:34:54AM +0200, Ard Biesheuvel wrote:
> On 28 June 2018 at 10:31, Peter Zijlstra <[email protected]> wrote:
> > On Wed, Jun 27, 2018 at 06:06:04PM +0200, Ard Biesheuvel wrote:
> >> Similar to the arm64 case, 64-bit x86 can benefit from using 32-bit
> >> relative references rather than 64-bit absolute ones when emitting
> >> struct jump_entry instances. Not only does this reduce the memory
> >> footprint of the entries themselves by 50%, it also removes the need
> >> for carrying relocation metadata on relocatable builds (i.e., for KASLR)
> >> which saves a fair chunk of .init space as well (although the savings
> >> are not as dramatic as on arm64)
> >
> > This will conflict with:
> >
> > https://lkml.kernel.org/r/[email protected]
>
> Thanks for the head's up. Fortunately, it does not conflict
> fundamentally, so it should be a straight-forward rebase after that
> code is merged.
Yeah, shouldn't be hard to cure.
There's another patch set that might have a little conflict, but that's
not near ready I think, so that'll have to just cope with things
shifting underneath (and there too, the fixup shouldn't be hard).
> Do you think this is likely to get merged for v4.19?
I'm thinking it is near ready so it might, but I'm not in charge of
those bits :-)
On 28 June 2018 at 11:25, Peter Zijlstra <[email protected]> wrote:
> On Thu, Jun 28, 2018 at 11:04:45AM +0200, Ard Biesheuvel wrote:
>> On 28 June 2018 at 11:02, Ard Biesheuvel <[email protected]> wrote:
>> >>> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>> >>>
>> >>> size = (((unsigned long)stop - (unsigned long)start)
>> >>> / sizeof(struct jump_entry));
>> >>> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
>> >>> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
>> >>> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
>> >>> + : NULL);
>> >>> }
>> >>
>> >> That will result in jump_label_swap being an unused symbol for some
>> >> compile options.
>> >
>> > No, and isn't that the point of IS_ENABLED()? The compiler sees a
>> > reference to jump_label_swap(), so it won't complain about it being
>> > unused.
>
> Ah, ok. I hadn't figured it was quite that smart about it.
>
Yeah. I could use a temp variable to make the indentation less
obnoxious, but since this is an opt-in feature, I'd like to preserve
the NULL (*swap)() argument for the existing users.
>> > Meh. I thought IS_ENABLED() was preferred over #ifdef, no?
>
> Dunno, I just reacted to the proposed code's uglyness :-)
>
I will try to come up with something that rhymes, ok? :-)
>> ... and it means the sort() routine will unconditionally perform an
>> indirect function call even if the arch does not require it.
>
> Yeah, not sure I care about that here, this is a one time affair, very
> far away from any fast paths.
>
Fair enough.
On Thu, Jun 28, 2018 at 11:29:40AM +0200, Ard Biesheuvel wrote:
> On 28 June 2018 at 11:25, Peter Zijlstra <[email protected]> wrote:
> >> > Meh. I thought IS_ENABLED() was preferred over #ifdef, no?
> >
> > Dunno, I just reacted to the proposed code's uglyness :-)
> >
>
> I will try to come up with something that rhymes, ok? :-)
:-)
> >> ... and it means the sort() routine will unconditionally perform an
> >> indirect function call even if the arch does not require it.
> >
> > Yeah, not sure I care about that here, this is a one time affair, very
> > far away from any fast paths.
> >
>
> Fair enough.
I recently had thoughts about doing the sort at link time, but then I
figured I didn't care enough to go write the tool to do that. The reason
was in that other patch-set that might conflict, that wants to use jump
labels _very_ early (for x86), so we had to lift the ideal nops stuff
and the cpu feature bits it relies upon to early code.
All in all the patch wasn't terrible and that made me completely loose
interest about doing the link-time thing again.
However, if someone is 'concerned' about the boot time performance, and
figures avoiding the sort is somehow worth it, they can look into it.
On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote:
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 86ec0652d3b1..aa203dffe72c 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -121,6 +121,32 @@ struct static_key {
> #include <asm/jump_label.h>
>
> #ifndef __ASSEMBLY__
> +#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
> +
> +struct jump_entry {
> + int code;
> + int target;
> + int key;
> +};
I much prefer you use 'u32' there.
> +static void jump_label_swap(void *a, void *b, int size)
> +{
> + long delta = (unsigned long)a - (unsigned long)b;
> + struct jump_entry *jea = a;
> + struct jump_entry *jeb = b;
> + struct jump_entry tmp = *jea;
> +
> + jea->code = jeb->code - delta;
> + jea->target = jeb->target - delta;
> + jea->key = jeb->key - delta;
> +
> + jeb->code = tmp.code + delta;
> + jeb->target = tmp.target + delta;
> + jeb->key = tmp.key + delta;
> +}
> +
> static void
> jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
> {
> @@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
>
> size = (((unsigned long)stop - (unsigned long)start)
> / sizeof(struct jump_entry));
> - sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
> + sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
> + IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
> + : NULL);
> }
That will result in jump_label_swap being an unused symbol for some
compile options.
Would it not be much nicer to write that like:
static void jump_label_swap(void *a, void *b, int size)
{
struct jump_entry *jea = a, *jeb = b;
#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
long delta = a - b;
jea->code += delta;
jea->target += delta;
jea->key += delta;
jeb->code -= delta;
jeb->target -= delta;
jeb->key -= delta;
#else
swap(*jea, *jeb);
}
And then unconditionally use jump_label_swap().
FYI, we noticed the following commit (built with gcc-7):
commit: b1ff47aacea95e5be1bedf2aee740395b52f4591 ("[PATCH 5/5] x86/kernel: jump_table: use relative references")
url: https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/add-support-for-relative-references-in-jump-tables/20180628-021246
in testcase: boot
on test machine: qemu-system-i386 -enable-kvm -m 360M
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-----------------------------------------------------+------------+------------+
| | 1843c4017f | b1ff47aace |
+-----------------------------------------------------+------------+------------+
| boot_successes | 57 | 46 |
| boot_failures | 2 | 14 |
| Mem-Info | 2 | 3 |
| invoked_oom-killer:gfp_mask=0x | 1 | 3 |
| Out_of_memory:Kill_process | 1 | 3 |
| WARNING:at_kernel/jump_label.c:#__jump_label_update | 0 | 11 |
| EIP:__jump_label_update | 0 | 11 |
+-----------------------------------------------------+------------+------------+
[ 43.154660] WARNING: CPU: 0 PID: 351 at kernel/jump_label.c:388 __jump_label_update+0x101/0x130
[ 43.172391] Modules linked in:
[ 43.176312] CPU: 0 PID: 351 Comm: trinity-main Not tainted 4.18.0-rc2-00124-gb1ff47a #206
[ 43.186389] EIP: __jump_label_update+0x101/0x130
[ 43.192131] Code: a5 bf fd ff 6a 01 31 c9 ba 01 00 00 00 b8 c0 02 cd b1 c6 05 ba 2e cb b1 01 e8 8b bf fd ff ff 33 68 8b 35 b2 b1 e8 cf 74 f3 ff <0f> 0b 6a 01 31 c9 ba 01 00 00 00 b8 a8 02 cd b1 e8 6a bf fd ff 83
[ 43.215879] EAX: 00000021 EBX: b1cb67b0 ECX: 00000000 EDX: 00000000
[ 43.223498] ESI: b1cb67b8 EDI: b1cb2fbc EBP: b89c9dc0 ESP: b89c9d9c
[ 43.231212] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010292
[ 43.239602] CR0: 80050033 CR2: 0805a000 CR3: 08979000 CR4: 00000690
[ 43.247344] Call Trace:
[ 43.250614] jump_label_update+0x95/0x120
[ 43.255705] static_key_slow_inc_cpuslocked+0xcd/0xe0
[ 43.261993] static_key_slow_inc+0xd/0x10
[ 43.266986] tracepoint_probe_register_prio+0x257/0x320
[ 43.273467] tracepoint_probe_register+0xf/0x20
[ 43.279104] trace_event_reg+0x90/0x100
[ 43.283964] perf_trace_init+0x222/0x280
[ 43.288833] perf_tp_event_init+0x1d/0x50
[ 43.293947] perf_try_init_event+0x27/0xb0
[ 43.299066] perf_event_alloc+0x757/0xb20
[ 43.304996] __do_sys_perf_event_open+0x3de/0xd60
[ 43.310932] sys_perf_event_open+0x17/0x20
[ 43.315362] do_int80_syscall_32+0x98/0x1f0
[ 43.319354] entry_INT80_32+0x33/0x33
[ 43.322816] EIP: 0xa7fa41b2
[ 43.325381] Code: 89 c2 31 c0 89 d7 f3 aa 8b 44 24 1c 89 30 c6 40 04 00 83 c4 2c 89 f0 5b 5e 5f 5d c3 90 90 90 90 90 90 90 90 90 90 90 90 cd 80 <c3> 8d b6 00 00 00 00 8d bc 27 00 00 00 00 8b 1c 24 c3 8d b6 00 00
[ 43.346022] EAX: ffffffda EBX: 080d3000 ECX: 0000015f EDX: ffffffff
[ 43.355195] ESI: ffffffff EDI: 00000001 EBP: 00000000 ESP: af819388
[ 43.362426] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000282
[ 43.368681] ---[ end trace 323a8199e30cb153 ]---
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
Xiaolong