Re: [PATCH v2 2/3] jump_label: mips: move module NOP patching into arch code

[Alexander Gordeev]

On Wed, Jun 15, 2022 at 05:41:41PM +0200, Ard Biesheuvel wrote:
> MIPS is the only remaining architecture that needs to patch jump label
> NOP encodings to initialize them at load time. So let's move the module
> patching part of that from generic code into arch/mips, and drop it from
> the others.
> 
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
>  arch/mips/kernel/jump_label.c | 19 ++++++++++++++
>  arch/mips/kernel/module.c     |  5 ++--
>  arch/s390/kernel/module.c     |  1 -
>  arch/sparc/kernel/module.c    |  3 ---
>  arch/x86/kernel/module.c      |  3 ---
>  include/linux/jump_label.h    |  7 +----
>  kernel/jump_label.c           | 27 +-------------------
>  7 files changed, 24 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/mips/kernel/jump_label.c b/arch/mips/kernel/jump_label.c
> index 662c8db9f45b..e4c775e7c063 100644
> --- a/arch/mips/kernel/jump_label.c
> +++ b/arch/mips/kernel/jump_label.c
> @@ -88,3 +88,22 @@ void arch_jump_label_transform(struct jump_entry *e,
>  
>  	mutex_unlock(&text_mutex);
>  }
> +
> +#ifdef CONFIG_MODULE
> +void jump_label_apply_nops(struct module *mod)
> +{
> +	struct jump_entry *iter_start = mod->jump_entries;
> +	struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
> +	struct jump_entry *iter;
> +
> +	/* if the module doesn't have jump label entries, just return */
> +	if (iter_start == iter_stop)
> +		return;
> +
> +	for (iter = iter_start; iter < iter_stop; iter++) {
> +		/* Only write NOPs for arch_branch_static(). */
> +		if (jump_label_init_type(iter) == JUMP_LABEL_NOP)
> +			arch_jump_label_transform(iter, JUMP_LABEL_NOP);
> +	}
> +}
> +#endif
> diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
> index 14f46d17500a..0c936cbf20c5 100644
> --- a/arch/mips/kernel/module.c
> +++ b/arch/mips/kernel/module.c
> @@ -21,6 +21,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/jump_label.h>
>  
> +extern void jump_label_apply_nops(struct module *mod);
>  
>  struct mips_hi16 {
>  	struct mips_hi16 *next;
> @@ -428,8 +429,8 @@ int module_finalize(const Elf_Ehdr *hdr,
>  	const Elf_Shdr *s;
>  	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
> -	/* Make jump label nops. */
> -	jump_label_apply_nops(me);
> +	if (IS_ENABLED(CONFIG_JUMP_LABEL))
> +		jump_label_apply_nops(me);
>  
>  	INIT_LIST_HEAD(&me->arch.dbe_list);
>  	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index 26125a9c436d..2d159b32885b 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -548,6 +548,5 @@ int module_finalize(const Elf_Ehdr *hdr,
>  #endif /* CONFIG_FUNCTION_TRACER */
>  	}
>  
> -	jump_label_apply_nops(me);
>  	return 0;
>  }
> diff --git a/arch/sparc/kernel/module.c b/arch/sparc/kernel/module.c
> index df39580f398d..66c45a2764bc 100644
> --- a/arch/sparc/kernel/module.c
> +++ b/arch/sparc/kernel/module.c
> @@ -208,9 +208,6 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
> -	/* make jump label nops */
> -	jump_label_apply_nops(me);
> -
>  	do_patch_sections(hdr, sechdrs);
>  
>  	/* Cheetah's I-cache is fully coherent.  */
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index b98ffcf4d250..95b9cf25d4bd 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -304,9 +304,6 @@ int module_finalize(const Elf_Ehdr *hdr,
>  					    tseg, tseg + text->sh_size);
>  	}
>  
> -	/* make jump label nops */
> -	jump_label_apply_nops(me);
> -
>  	if (orc && orc_ip)
>  		unwind_module_init(me, (void *)orc_ip->sh_addr, orc_ip->sh_size,
>  				   (void *)orc->sh_addr, orc->sh_size);
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index bf1eef337a07..2003a0935478 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -230,12 +230,12 @@ extern void static_key_slow_inc(struct static_key *key);
>  extern void static_key_slow_dec(struct static_key *key);
>  extern void static_key_slow_inc_cpuslocked(struct static_key *key);
>  extern void static_key_slow_dec_cpuslocked(struct static_key *key);
> -extern void jump_label_apply_nops(struct module *mod);
>  extern int static_key_count(struct static_key *key);
>  extern void static_key_enable(struct static_key *key);
>  extern void static_key_disable(struct static_key *key);
>  extern void static_key_enable_cpuslocked(struct static_key *key);
>  extern void static_key_disable_cpuslocked(struct static_key *key);
> +extern enum jump_label_type jump_label_init_type(struct jump_entry *entry);
>  
>  /*
>   * We should be using ATOMIC_INIT() for initializing .enabled, but
> @@ -303,11 +303,6 @@ static inline int jump_label_text_reserved(void *start, void *end)
>  static inline void jump_label_lock(void) {}
>  static inline void jump_label_unlock(void) {}
>  
> -static inline int jump_label_apply_nops(struct module *mod)
> -{
> -	return 0;
> -}
> -
>  static inline void static_key_enable(struct static_key *key)
>  {
>  	STATIC_KEY_CHECK_USE(key);
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index b156e152d6b4..b1ac2948be79 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -508,7 +508,7 @@ void __init jump_label_init(void)
>  
>  #ifdef CONFIG_MODULES
>  
> -static enum jump_label_type jump_label_init_type(struct jump_entry *entry)
> +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);
> @@ -596,31 +596,6 @@ static void __jump_label_mod_update(struct static_key *key)
>  	}
>  }
>  
> -/***
> - * apply_jump_label_nops - patch module jump labels with arch_get_jump_label_nop()
> - * @mod: module to patch
> - *
> - * Allow for run-time selection of the optimal nops. Before the module
> - * loads patch these with arch_get_jump_label_nop(), which is specified by
> - * the arch specific jump label code.
> - */
> -void jump_label_apply_nops(struct module *mod)
> -{
> -	struct jump_entry *iter_start = mod->jump_entries;
> -	struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
> -	struct jump_entry *iter;
> -
> -	/* if the module doesn't have jump label entries, just return */
> -	if (iter_start == iter_stop)
> -		return;
> -
> -	for (iter = iter_start; iter < iter_stop; iter++) {
> -		/* Only write NOPs for arch_branch_static(). */
> -		if (jump_label_init_type(iter) == JUMP_LABEL_NOP)
> -			arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
> -	}
> -}
> -
>  static int jump_label_add_module(struct module *mod)
>  {
>  	struct jump_entry *iter_start = mod->jump_entries;

To the common and s390 parts:

Acked-by: Alexander Gordeev <agordeev@xxxxxxxxxxxxx>

FWIW, the change is not mips-only, thus Subject is not exactly correct.

> -- 
> 2.35.1
>