Re: [RFC PATCH v2 03/13] module: Move livepatch support to a separate file

Re: [RFC PATCH v2 03/13] module: Move livepatch support to a separate file

[Petr Mladek]

On Thu 2022-01-06 23:43:09, Aaron Tomlin wrote:
> No functional change.
> 
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  kernel/module/Makefile    |  1 +
>  kernel/module/internal.h  | 12 ++++++
>  kernel/module/livepatch.c | 75 +++++++++++++++++++++++++++++++++
>  kernel/module/main.c      | 89 +--------------------------------------
>  4 files changed, 89 insertions(+), 88 deletions(-)
>  create mode 100644 kernel/module/livepatch.c
> 
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index a9cf6e822075..47d70bb18da3 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -6,3 +6,4 @@
>  obj-$(CONFIG_MODULES) += main.o
>  obj-$(CONFIG_MODULE_SIG) += signing.o
>  obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index ffc50df010a7..91ef152aeffb 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -51,3 +51,15 @@ struct load_info {
>  };
>  
>  extern int mod_verify_sig(const void *mod, struct load_info *info);
> +
> +#ifdef CONFIG_LIVEPATCH
> +extern int copy_module_elf(struct module *mod, struct load_info *info);
> +extern void free_module_elf(struct module *mod);
> +extern int check_modinfo_livepatch(struct module *mod, struct load_info *info);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	return 0;
> +}
> +static inline void free_module_elf(struct module *mod) { }

It looks like there is no check_modinfo_livepatch() variant when
CONFIG_LIPATCH is disabled.

> +#endif /* CONFIG_LIVEPATCH */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 2a6b859716c0..9bcaf251e109 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3052,19 +2977,7 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
>  	return 0;
>  }
>  
> -#ifdef CONFIG_LIVEPATCH
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> -	if (get_modinfo(info, "livepatch")) {
> -		mod->klp = true;
> -		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> -		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
> -			       mod->name);
 > -	}
> -
> -	return 0;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> +#ifndef CONFIG_LIVEPATCH
>  static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
>  {
>  	if (get_modinfo(info, "livepatch")) {

But it exist here.

It would be better to have the two variants close each other. I mean
to have it somewhere like:

#ifdef CONFIG_LIVEPATCH

   variant A

#else

   variant B

#endif


A solution would be to do it a similar way like in
check_modinfo_retpoline(). Have a generic:

static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
{
	if (!get_modinfo(info, "livepatch"))
		return 0;

	if (set_livepatch_module(mod)) {
		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
			       mod->name);
		return 0;
       }

       pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
	       mod->name);
       return -ENOEXEC;
}

, where set_livepatch_module(mod) might be defined inline
similar way like is_livepatch_module():

#ifdef CONFIG_LIVEPATCH
static inline bool set_livepatch_module(struct module *mod)
{
	mod->klp = true;
	return true;
}
#else /* !CONFIG_LIVEPATCH */
static inline bool set_livepatch_module(struct module *mod)
{
	return false;
}
#endif /* CONFIG_LIVEPATCH */


Well, it might be matter of taste. Others might prefer another solution.
Adding live-patching mailing list into Cc.

Anyway, if we do any code refactoring, we should do it in a separate
preparatory patch.

Best Regards,
Petr

Re: [RFC PATCH v2 03/13] module: Move livepatch support to a separate file

[David Vernet]

Petr Mladek <pmladek@suse.com> wrote on Wed [2022-Jan-12 17:53:56 +0100]:
> It would be better to have the two variants close each other. I mean
> to have it somewhere like:
> 
> #ifdef CONFIG_LIVEPATCH
> 
>    variant A
> 
> #else
> 
>    variant B
> 
> #endif
> 

<snip>

> #ifdef CONFIG_LIVEPATCH
> static inline bool set_livepatch_module(struct module *mod)
> {
> 	mod->klp = true;
> 	return true;
> }
> #else /* !CONFIG_LIVEPATCH */
> static inline bool set_livepatch_module(struct module *mod)
> {
> 	return false;
> }
> #endif /* CONFIG_LIVEPATCH */
> 
> 
> Well, it might be matter of taste. Others might prefer another solution.
> Adding live-patching mailing list into Cc.

+1 -- this seems like a cleaner approach.

- David

Re: [RFC PATCH v2 03/13] module: Move livepatch support to a separate file

[Aaron Tomlin]

On Wed 2022-01-12 17:53 +0100, Petr Mladek wrote:
> It would be better to have the two variants close each other. I mean
> to have it somewhere like:
> 
> #ifdef CONFIG_LIVEPATCH
> 
>    variant A
> 
> #else
> 
>    variant B
> 
> #endif

Petr,

I agree. I'll incorporate this approach into RFC PATCH v3.


Kind regards,

-- 
Aaron Tomlin