Re: [PATCH v5 08/10] livepatch: Improve the search performance of module_kallsyms_on_each_symbol()

[Leizhen (ThunderTown)]

On 2022/9/23 19:20, Zhen Lei wrote:
> Currently we traverse all symbols of all modules to find the specified
> function for the specified module. But in reality, we just need to find
> the given module and then traverse all the symbols in it.
> 
> In order to achieve this purpose, split the call to hook 'fn' into two
> phases:
> 1. Finds the given module. Pass pointer 'mod'. Hook 'fn' directly returns
>    the comparison result of the module name without comparing the function
>    name.
> 2. Finds the given function in that module. Pass pointer 'mod = NULL'.
>    Hook 'fn' skip the comparison of module name and directly compare
>    function names.

Sorry, I forgot to change the description. I will fix it in v6, after I've
collected review comments.

> 
> Phase1: mod1-->mod2..(subsequent modules do not need to be compared)
>                 |
> Phase2:          -->f1-->f2-->f3
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> ---
>  include/linux/module.h   |  4 ++--
>  kernel/livepatch/core.c  | 13 ++-----------
>  kernel/module/kallsyms.c | 15 ++++++++++++---
>  3 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 518296ea7f73af6..6e1a531d78e7e8b 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -879,8 +879,8 @@ static inline bool module_sig_ok(struct module *module)
>  }
>  #endif	/* CONFIG_MODULE_SIG */
>  
> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> -					     struct module *, unsigned long),
> +int module_kallsyms_on_each_symbol(const char *modname,
> +				   int (*fn)(void *, const char *, unsigned long),
>  				   void *data);
>  
>  #endif /* _LINUX_MODULE_H */
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 31b57ccf908017e..b02de4cb311c703 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -118,27 +118,19 @@ static struct klp_object *klp_find_object(struct klp_patch *patch,
>  }
>  
>  struct klp_find_arg {
> -	const char *objname;
>  	const char *name;
>  	unsigned long addr;
>  	unsigned long count;
>  	unsigned long pos;
>  };
>  
> -static int klp_find_callback(void *data, const char *name,
> -			     struct module *mod, unsigned long addr)
> +static int klp_find_callback(void *data, const char *name, unsigned long addr)
>  {
>  	struct klp_find_arg *args = data;
>  
> -	if ((mod && !args->objname) || (!mod && args->objname))
> -		return 0;
> -
>  	if (strcmp(args->name, name))
>  		return 0;
>  
> -	if (args->objname && strcmp(args->objname, mod->name))
> -		return 0;
> -
>  	args->addr = addr;
>  	args->count++;
>  
> @@ -175,7 +167,6 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  				  unsigned long sympos, unsigned long *addr)
>  {
>  	struct klp_find_arg args = {
> -		.objname = objname,
>  		.name = name,
>  		.addr = 0,
>  		.count = 0,
> @@ -183,7 +174,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>  	};
>  
>  	if (objname)
> -		module_kallsyms_on_each_symbol(klp_find_callback, &args);
> +		module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
>  	else
>  		kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
>  
> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> index f5c5c9175333df7..329cef573675d49 100644
> --- a/kernel/module/kallsyms.c
> +++ b/kernel/module/kallsyms.c
> @@ -495,8 +495,8 @@ unsigned long module_kallsyms_lookup_name(const char *name)
>  }
>  
>  #ifdef CONFIG_LIVEPATCH
> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> -					     struct module *, unsigned long),
> +int module_kallsyms_on_each_symbol(const char *modname,
> +				   int (*fn)(void *, const char *, unsigned long),
>  				   void *data)
>  {
>  	struct module *mod;
> @@ -510,6 +510,9 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
>  		if (mod->state == MODULE_STATE_UNFORMED)
>  			continue;
>  
> +		if (strcmp(modname, mod->name))
> +			continue;
> +
>  		/* Use rcu_dereference_sched() to remain compliant with the sparse tool */
>  		preempt_disable();
>  		kallsyms = rcu_dereference_sched(mod->kallsyms);
> @@ -522,10 +525,16 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
>  				continue;
>  
>  			ret = fn(data, kallsyms_symbol_name(kallsyms, i),
> -				 mod, kallsyms_symbol_value(sym));
> +				 kallsyms_symbol_value(sym));
>  			if (ret != 0)
>  				goto out;
>  		}
> +
> +		/*
> +		 * The given module is found, the subsequent modules do not
> +		 * need to be compared.
> +		 */
> +		break;
>  	}
>  out:
>  	mutex_unlock(&module_mutex);
> 

-- 
Regards,
  Zhen Lei