Re: [PATCH v5 04/13] module: Move livepatch support to a separate file

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Aaron Tomlin <atomlin@redhat.com>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>
Cc: "cl@linux.com" <cl@linux.com>,
	"pmladek@suse.com" <pmladek@suse.com>,
	"mbenes@suse.cz" <mbenes@suse.cz>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"jeyu@kernel.org" <jeyu@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"live-patching@vger.kernel.org" <live-patching@vger.kernel.org>,
	"atomlin@atomlin.com" <atomlin@atomlin.com>,
	"ghalat@redhat.com" <ghalat@redhat.com>,
	"allen.lkml@gmail.com" <allen.lkml@gmail.com>,
	"void@manifault.com" <void@manifault.com>,
	"joe@perches.com" <joe@perches.com>,
	"msuchanek@suse.de" <msuchanek@suse.de>,
	"oleksandr@natalenko.name" <oleksandr@natalenko.name>
Subject: Re: [PATCH v5 04/13] module: Move livepatch support to a separate file
Date: Thu, 10 Feb 2022 11:44:52 +0000	[thread overview]
Message-ID: <c8f97323-fcaa-5316-df79-60fd91c837aa@csgroup.eu> (raw)
In-Reply-To: <20220209170358.3266629-5-atomlin@redhat.com>

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> 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>
> ---
>   include/linux/module.h    |   5 +-
>   kernel/module/Makefile    |   3 ++
>   kernel/module/internal.h  |  18 +++++++
>   kernel/module/livepatch.c |  80 ++++++++++++++++++++++++++++++
>   kernel/module/main.c      | 102 ++++----------------------------------
>   5 files changed, 112 insertions(+), 96 deletions(-)
>   create mode 100644 kernel/module/livepatch.c
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e135fd5c076..680b31ff57fa 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
>   }
>   
>   #ifdef CONFIG_LIVEPATCH
> -static inline bool is_livepatch_module(struct module *mod)
> -{
> -	return mod->klp;
> -}
> +bool is_livepatch_module(struct module *mod);

This change is wrong, build fails with it because is_livepatch_module() 
is nowhere defined.

You could move is_livepatch_module() to include/linux/livepatch.h but as 
a separate patch.

>   #else /* !CONFIG_LIVEPATCH */
>   static inline bool is_livepatch_module(struct module *mod)
>   {
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 2902fc7d0ef1..ee20d864ad19 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
>   obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
>   obj-$(CONFIG_MODULE_SIG) += signing.o
>   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +ifdef CONFIG_MODULES

CONFIG_LIVEPATCH depends on CONFIG_MODULES so this ifdef is not needed 
(See kernel/livepatch/Kconfig)

> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1cf5d6dabc97..d252e0af1c54 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -58,6 +58,24 @@ struct load_info {
>   
>   int mod_verify_sig(const void *mod, struct load_info *info);
>   
> +#ifdef CONFIG_LIVEPATCH
> +int copy_module_elf(struct module *mod, struct load_info *info);
> +void free_module_elf(struct module *mod);
> +bool set_livepatch_module(struct module *mod);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	return 0;
> +}
> +
> +static inline bool set_livepatch_module(struct module *mod)
> +{
> +	return false;
> +}
> +
> +static inline void free_module_elf(struct module *mod) { }
> +#endif /* CONFIG_LIVEPATCH */
> +
>   #ifdef CONFIG_MODULE_DECOMPRESS
>   int module_decompress(struct load_info *info, const void *buf, size_t size);
>   void module_decompress_cleanup(struct load_info *info);
> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..7e9cf530c3f0
> --- /dev/null
> +++ b/kernel/module/livepatch.c

Checkpatch reports the following:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#80:
new file mode 100644

CHECK: Comparison to NULL could be written "!mod->klp_info"
#109: FILE: kernel/module/livepatch.c:25:
+	if (mod->klp_info == NULL)

CHECK: Comparison to NULL could be written "!mod->klp_info->sechdrs"
#119: FILE: kernel/module/livepatch.c:35:
+	if (mod->klp_info->sechdrs == NULL) {

CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#127: FILE: kernel/module/livepatch.c:43:
+	if (mod->klp_info->secstrings == NULL) {

CHECK: No space is necessary after a cast
#142: FILE: kernel/module/livepatch.c:58:
+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) 
mod->core_kallsyms.symtab;

> +inline bool set_livepatch_module(struct module *mod)

'inline' keyword is pointless here, as far as this function is in a .c 
and is not static, it won't be inlined.

Given how simple this function is, it should be a 'static inline' in 
internal.c

> +{
> +	mod->klp = true;
> +	return true;
> +}

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 750e3ad28679..5f5bd7152b55 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c

> @@ -3091,30 +3016,23 @@ 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;
> +	if (!get_modinfo(info, "livepatch"))
> +		/* Nothing more to do */
> +		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;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> -	if (get_modinfo(info, "livepatch")) {
> -		pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> -		       mod->name);
> -		return -ENOEXEC;
> +				mod->name);

This change seems wrong, mod->name must remain aligned to open parenthesis.

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

CHECK: Alignment should match open parenthesis
#285: FILE: kernel/module/main.c:3033:
+	pr_err("%s: module is marked as livepatch module, but livepatch 
support is disabled",
+		mod->name);

> +	return -ENOEXEC;
>   }
> -#endif /* CONFIG_LIVEPATCH */
>   
>   static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
>   {