Re: [PATCH v6 8/8] x86/kernel: jump_table: use relative references

2017-12-28 Thread Steven Rostedt 
On Thu, 28 Dec 2017 16:26:07 +
Ard Biesheuvel  wrote:

> On 28 December 2017 at 16:19, Steven Rostedt  wrote:
> > On Wed, 27 Dec 2017 08:50:33 +
> > Ard Biesheuvel  wrote:
> >  
> >>  static inline jump_label_t jump_entry_code(const struct jump_entry *entry)
> >>  {
> >> - return entry->code;
> >> + return (jump_label_t)>code + entry->code;  
> >
> > I'm paranoid about doing arithmetic on abstract types. What happens in
> > the future if jump_label_t becomes a pointer? You will get a different
> > result.
> >  
> 
> In general, I share your concern. In this case, however, jump_label_t
> is typedef'd three lines up and is never used anywhere else.

I would agree if this was in a .c file, but it's in a header file,
which causes me to be more paranoid.

> 
> > Could we switch these calculations to something like:
> >
> > return (jump_label_t)((long)>code + entry->code);
> >  
> 
> jump_label_t is local to this .h file, so it can be defined as u32 or
> u64 depending on the word size. I don't mind adding the extra cast,
> but I am not sure if your paranoia is justified in this particular
> case. Perhaps we should just use 'unsigned long' throughout?

Actually, that may be better. Have the return value be jump_label_t,
but the cast be "unsigned long". That way it should always work.

static inline jump_label_t jump_entry_code(...)
{
return (unsigned long)>code + entry->code;
}


-- Steve

Re: [PATCH v6 8/8] x86/kernel: jump_table: use relative references

2017-12-28 Thread Ard Biesheuvel 
On 28 December 2017 at 16:19, Steven Rostedt  wrote:
> On Wed, 27 Dec 2017 08:50:33 +
> Ard Biesheuvel  wrote:
>
>>  static inline jump_label_t jump_entry_code(const struct jump_entry *entry)
>>  {
>> - return entry->code;
>> + return (jump_label_t)>code + entry->code;
>
> I'm paranoid about doing arithmetic on abstract types. What happens in
> the future if jump_label_t becomes a pointer? You will get a different
> result.
>

In general, I share your concern. In this case, however, jump_label_t
is typedef'd three lines up and is never used anywhere else.

> Could we switch these calculations to something like:
>
> return (jump_label_t)((long)>code + entry->code);
>

jump_label_t is local to this .h file, so it can be defined as u32 or
u64 depending on the word size. I don't mind adding the extra cast,
but I am not sure if your paranoia is justified in this particular
case. Perhaps we should just use 'unsigned long' throughout?

>> +}
>> +
>> +static inline jump_label_t jump_entry_target(const struct jump_entry *entry)
>> +{
>> + return (jump_label_t)>target + entry->target;
>>  }
>>
>>  static inline struct static_key *jump_entry_key(const struct jump_entry 
>> *entry)
>>  {
>> - return (struct static_key *)((unsigned long)entry->key & ~1UL);
>> + unsigned long key = (unsigned long)>key + entry->key;
>> +
>> + return (struct static_key *)(key & ~1UL);
>>  }
>>
>>  static inline bool jump_entry_is_branch(const struct jump_entry *entry)
>> @@ -99,7 +106,7 @@ static inline void jump_entry_set_module_init(struct 
>> jump_entry *entry)
>>   entry->code = 0;
>>  }
>>
>> -#define jump_label_swap  NULL
>> +void jump_label_swap(void *a, void *b, int size);
>>
>>  #else/* __ASSEMBLY__ */
>>
>> @@ -114,8 +121,8 @@ static inline void jump_entry_set_module_init(struct 
>> jump_entry *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
>>
>> @@ -130,8 +137,8 @@ static inline void jump_entry_set_module_init(struct 
>> jump_entry *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/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
>> index e56c95be2808..cc5034b42335 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),
>
> You have the functions already made before this patch. Perhaps we
> should have a separate patch to use them (here and elsewhere) before
> you make the conversion to using relative references. It will help out
> in debugging and bisects. To know if the use of functions is an issue,
> or the conversion of relative references is an issue.
>
> I suggest splitting this into two patches.
>

Fair enough.


>> +__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

Re: [PATCH v6 8/8] x86/kernel: jump_table: use relative references

2017-12-28 Thread Steven Rostedt 
On Wed, 27 Dec 2017 08:50:33 +
Ard Biesheuvel  wrote:

>  static inline jump_label_t jump_entry_code(const struct jump_entry *entry)
>  {
> - return entry->code;
> + return (jump_label_t)>code + entry->code;

I'm paranoid about doing arithmetic on abstract types. What happens in
the future if jump_label_t becomes a pointer? You will get a different
result.

Could we switch these calculations to something like:

return (jump_label_t)((long)>code + entry->code);

> +}
> +
> +static inline jump_label_t jump_entry_target(const struct jump_entry *entry)
> +{
> + return (jump_label_t)>target + entry->target;
>  }
>  
>  static inline struct static_key *jump_entry_key(const struct jump_entry 
> *entry)
>  {
> - return (struct static_key *)((unsigned long)entry->key & ~1UL);
> + unsigned long key = (unsigned long)>key + entry->key;
> +
> + return (struct static_key *)(key & ~1UL);
>  }
>  
>  static inline bool jump_entry_is_branch(const struct jump_entry *entry)
> @@ -99,7 +106,7 @@ static inline void jump_entry_set_module_init(struct 
> jump_entry *entry)
>   entry->code = 0;
>  }
>  
> -#define jump_label_swap  NULL
> +void jump_label_swap(void *a, void *b, int size);
>  
>  #else/* __ASSEMBLY__ */
>  
> @@ -114,8 +121,8 @@ static inline void jump_entry_set_module_init(struct 
> jump_entry *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
>  
> @@ -130,8 +137,8 @@ static inline void jump_entry_set_module_init(struct 
> jump_entry *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/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> index e56c95be2808..cc5034b42335 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),

You have the functions already made before this patch. Perhaps we
should have a separate patch to use them (here and elsewhere) before
you make the conversion to using relative references. It will help out
in debugging and bisects. To know if the use of functions is an issue,
or the conversion of relative references is an issue.

I suggest splitting this into two patches.

-- Steve


> +__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))
> +

[PATCH v6 8/8] x86/kernel: jump_table: use relative references

2017-12-27 Thread Ard Biesheuvel 
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 
---
 arch/x86/include/asm/jump_label.h | 35 +++-
 arch/x86/kernel/jump_label.c  | 59 ++--
 tools/objtool/special.c   |  4 +-
 3 files changed, 65 insertions(+), 33 deletions(-)

diff --git a/arch/x86/include/asm/jump_label.h 
b/arch/x86/include/asm/jump_label.h
index 009ff2699d07..91c01af96907 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);
 
@@ -69,19 +69,26 @@ typedef u32 jump_label_t;
 #endif
 
 struct jump_entry {
-   jump_label_t code;
-   jump_label_t target;
-   jump_label_t key;
+   s32 code;
+   s32 target;
+   s32 key;
 };
 
 static inline jump_label_t jump_entry_code(const struct jump_entry *entry)
 {
-   return entry->code;
+   return (jump_label_t)>code + entry->code;
+}
+
+static inline jump_label_t jump_entry_target(const struct jump_entry *entry)
+{
+   return (jump_label_t)>target + entry->target;
 }
 
 static inline struct static_key *jump_entry_key(const struct jump_entry *entry)
 {
-   return (struct static_key *)((unsigned long)entry->key & ~1UL);
+   unsigned long key = (unsigned long)>key + entry->key;
+
+   return (struct static_key *)(key & ~1UL);
 }
 
 static inline bool jump_entry_is_branch(const struct jump_entry *entry)
@@ -99,7 +106,7 @@ static inline void jump_entry_set_module_init(struct 
jump_entry *entry)
entry->code = 0;
 }
 
-#define jump_label_swapNULL
+void jump_label_swap(void *a, void *b, int size);
 
 #else  /* __ASSEMBLY__ */
 
@@ -114,8 +121,8 @@ static inline void jump_entry_set_module_init(struct 
jump_entry *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
 
@@ -130,8 +137,8 @@ static inline void jump_entry_set_module_init(struct 
jump_entry *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/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index e56c95be2808..cc5034b42335 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