[PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Julian Pidancet]

The alternatives_smp_module_add() function restricts patching of SMP
lock prefixes to the text address range passed as an argument.

For vmlinux, patching all the instructions located between the _text and
_etext symbols is allowed. That includes the .text section but also
other sections such as .text.hot and .text.unlikely.

As per the comment inside the 'struct smp_alt_module' definition, the
original purpose of this restriction is to avoid patching the init code
which may have been deallocated when the alternatives code run.

For modules, the current code only allows patching instructions located
inside the .text segment, excluding other sections such as .text.hot or
.text.unlikely, which may need patching.

This change aims to make patching of the kernel core and modules more
consistent, by allowing all text sections of modules except .init.text
to be patched in module_finalize().

For that we use mod->core_layout.base/mod->core_layout.text_size as the
address range allowed to be patched, which include all the code sections
except the init code.

Signed-off-by: Julian Pidancet <julian.pidancet@oracle.com>
---
Public tests: https://gist.github.com/jpidancet/1ee457623426f3e3902a28edaf2c80d0 
Related thread: https://marc.info/?t=130864398400006

 arch/x86/kernel/module.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index b1abf663417c..da22193eb5e0 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -251,14 +251,12 @@ int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
-	const Elf_Shdr *s, *text = NULL, *alt = NULL, *locks = NULL,
-		*para = NULL, *orc = NULL, *orc_ip = NULL,
-		*retpolines = NULL, *returns = NULL, *ibt_endbr = NULL;
+	const Elf_Shdr *s, *alt = NULL, *locks = NULL, *para = NULL,
+		*orc = NULL, *orc_ip = NULL, *retpolines = NULL,
+		*returns = NULL, *ibt_endbr = NULL;
 	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
 	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
-		if (!strcmp(".text", secstrings + s->sh_name))
-			text = s;
 		if (!strcmp(".altinstructions", secstrings + s->sh_name))
 			alt = s;
 		if (!strcmp(".smp_locks", secstrings + s->sh_name))
@@ -302,12 +300,13 @@ int module_finalize(const Elf_Ehdr *hdr,
 		void *iseg = (void *)ibt_endbr->sh_addr;
 		apply_ibt_endbr(iseg, iseg + ibt_endbr->sh_size);
 	}
-	if (locks && text) {
+	if (locks) {
 		void *lseg = (void *)locks->sh_addr;
-		void *tseg = (void *)text->sh_addr;
+		void *text = me->core_layout.base;
+		void *text_end = text + me->core_layout.text_size;
 		alternatives_smp_module_add(me, me->name,
 					    lseg, lseg + locks->sh_size,
-					    tseg, tseg + text->sh_size);
+					    text, text_end);
 	}
 
 	if (orc && orc_ip)
-- 
2.37.1

Re: [PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Julian Pidancet]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

Hi list,

Kind ping.

On Tue Aug 30, 2022 at 09:42, Julian Pidancet wrote:
> The alternatives_smp_module_add() function restricts patching of SMP
> lock prefixes to the text address range passed as an argument.
>
> For vmlinux, patching all the instructions located between the _text and
> _etext symbols is allowed. That includes the .text section but also
> other sections such as .text.hot and .text.unlikely.
>
> As per the comment inside the 'struct smp_alt_module' definition, the
> original purpose of this restriction is to avoid patching the init code
> which may have been deallocated when the alternatives code run.
>
> For modules, the current code only allows patching instructions located
> inside the .text segment, excluding other sections such as .text.hot or
> .text.unlikely, which may need patching.
>
> This change aims to make patching of the kernel core and modules more
> consistent, by allowing all text sections of modules except .init.text
> to be patched in module_finalize().
>
> For that we use mod->core_layout.base/mod->core_layout.text_size as the
> address range allowed to be patched, which include all the code sections
> except the init code.
>
> Signed-off-by: Julian Pidancet <julian.pidancet@oracle.com>
> ---
> Public tests: https://gist.github.com/jpidancet/1ee457623426f3e3902a28edaf2c80d0 
> Related thread: https://marc.info/?t=130864398400006
>
>  arch/x86/kernel/module.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index b1abf663417c..da22193eb5e0 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -251,14 +251,12 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
> -	const Elf_Shdr *s, *text = NULL, *alt = NULL, *locks = NULL,
> -		*para = NULL, *orc = NULL, *orc_ip = NULL,
> -		*retpolines = NULL, *returns = NULL, *ibt_endbr = NULL;
> +	const Elf_Shdr *s, *alt = NULL, *locks = NULL, *para = NULL,
> +		*orc = NULL, *orc_ip = NULL, *retpolines = NULL,
> +		*returns = NULL, *ibt_endbr = NULL;
>  	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
>  	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
> -		if (!strcmp(".text", secstrings + s->sh_name))
> -			text = s;
>  		if (!strcmp(".altinstructions", secstrings + s->sh_name))
>  			alt = s;
>  		if (!strcmp(".smp_locks", secstrings + s->sh_name))
> @@ -302,12 +300,13 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		void *iseg = (void *)ibt_endbr->sh_addr;
>  		apply_ibt_endbr(iseg, iseg + ibt_endbr->sh_size);
>  	}
> -	if (locks && text) {
> +	if (locks) {
>  		void *lseg = (void *)locks->sh_addr;
> -		void *tseg = (void *)text->sh_addr;
> +		void *text = me->core_layout.base;
> +		void *text_end = text + me->core_layout.text_size;
>  		alternatives_smp_module_add(me, me->name,
>  					    lseg, lseg + locks->sh_size,
> -					    tseg, tseg + text->sh_size);
> +					    text, text_end);
>  	}
>  
>  	if (orc && orc_ip)
> -- 
> 2.37.1

-- 
Julian


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Borislav Petkov]

On Tue, Aug 30, 2022 at 09:42:37AM +0200, Julian Pidancet wrote:
> The alternatives_smp_module_add() function restricts patching of SMP
> lock prefixes to the text address range passed as an argument.
> 
> For vmlinux, patching all the instructions located between the _text and
> _etext symbols is allowed. That includes the .text section but also
> other sections such as .text.hot and .text.unlikely.
> 
> As per the comment inside the 'struct smp_alt_module' definition, the
> original purpose of this restriction is to avoid patching the init code
> which may have been deallocated when the alternatives code run.
> 
> For modules, the current code only allows patching instructions located
> inside the .text segment, excluding other sections such as .text.hot or
> .text.unlikely, which may need patching.

Is this something you noticed by inspection or is there a real failure
behind it?

> This change aims to make patching of the kernel core and modules more

Avoid having "This patch" or "This commit" and so on, in the commit
message. It is tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> consistent, by allowing all text sections of modules except .init.text
> to be patched in module_finalize().
> 
> For that we use mod->core_layout.base/mod->core_layout.text_size as the

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> address range allowed to be patched, which include all the code sections
> except the init code.
> 
> Signed-off-by: Julian Pidancet <julian.pidancet@oracle.com>
> ---
> Public tests: https://gist.github.com/jpidancet/1ee457623426f3e3902a28edaf2c80d0 

Looks like you wrote a module to verify that :)

> Related thread: https://marc.info/?t=130864398400006

Aha, someone else noticed this inconsistency.

>  arch/x86/kernel/module.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index b1abf663417c..da22193eb5e0 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -251,14 +251,12 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		    const Elf_Shdr *sechdrs,
>  		    struct module *me)
>  {
> -	const Elf_Shdr *s, *text = NULL, *alt = NULL, *locks = NULL,
> -		*para = NULL, *orc = NULL, *orc_ip = NULL,
> -		*retpolines = NULL, *returns = NULL, *ibt_endbr = NULL;
> +	const Elf_Shdr *s, *alt = NULL, *locks = NULL, *para = NULL,
> +		*orc = NULL, *orc_ip = NULL, *retpolines = NULL,
> +		*returns = NULL, *ibt_endbr = NULL;
>  	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  
>  	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {
> -		if (!strcmp(".text", secstrings + s->sh_name))
> -			text = s;
>  		if (!strcmp(".altinstructions", secstrings + s->sh_name))
>  			alt = s;
>  		if (!strcmp(".smp_locks", secstrings + s->sh_name))
> @@ -302,12 +300,13 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		void *iseg = (void *)ibt_endbr->sh_addr;
>  		apply_ibt_endbr(iseg, iseg + ibt_endbr->sh_size);
>  	}
> -	if (locks && text) {
> +	if (locks) {
>  		void *lseg = (void *)locks->sh_addr;
> -		void *tseg = (void *)text->sh_addr;
> +		void *text = me->core_layout.base;
> +		void *text_end = text + me->core_layout.text_size;
>  		alternatives_smp_module_add(me, me->name,
>  					    lseg, lseg + locks->sh_size,
> -					    tseg, tseg + text->sh_size);
> +					    text, text_end);
>  	}
>  
>  	if (orc && orc_ip)
> -- 

I don't see anything wrong with doing that on a quick glance...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Peter Zijlstra]

On Tue, Aug 30, 2022 at 09:42:37AM +0200, Julian Pidancet wrote:
> The alternatives_smp_module_add() function restricts patching of SMP
> lock prefixes to the text address range passed as an argument.
> 
> For vmlinux, patching all the instructions located between the _text and
> _etext symbols is allowed. That includes the .text section but also
> other sections such as .text.hot and .text.unlikely.
> 
> As per the comment inside the 'struct smp_alt_module' definition, the
> original purpose of this restriction is to avoid patching the init code
> which may have been deallocated when the alternatives code run.

It is not; init code is still around when we run alternatives.

Re: [PATCH] x86/alternative: Consistently patch SMP locks in vmlinux and modules

[Julian Pidancet]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

On Wed Oct 26, 2022 at 20:08, Borislav Petkov wrote:
> On Tue, Aug 30, 2022 at 09:42:37AM +0200, Julian Pidancet wrote:
> > The alternatives_smp_module_add() function restricts patching of SMP
> > lock prefixes to the text address range passed as an argument.
> >
> > For vmlinux, patching all the instructions located between the _text and
> > _etext symbols is allowed. That includes the .text section but also
> > other sections such as .text.hot and .text.unlikely.
> >
> > As per the comment inside the 'struct smp_alt_module' definition, the
> > original purpose of this restriction is to avoid patching the init code
> > which may have been deallocated when the alternatives code run.
> >
> > For modules, the current code only allows patching instructions located
> > inside the .text segment, excluding other sections such as .text.hot or
> > .text.unlikely, which may need patching.
>
> Is this something you noticed by inspection or is there a real failure
> behind it?
>

We do live patching of the kernel code at Ksplice. Before applying a
binary patch we have a safety step that compares the running code with
the code that the binary patch is expected to modify and abort if they
differ.
This problem was picked up by the tools that we developped for this
purpose. We have a workaround for this internally, yet the inconsistency
lies in the kernel, so we thought it best fixed there directly.

> > This change aims to make patching of the kernel core and modules more
>
> Avoid having "This patch" or "This commit" and so on, in the commit
> message. It is tautologically useless.
>
> Also, do
>
> $ git grep 'This patch' Documentation/process
>
> for more details.
>
> > consistent, by allowing all text sections of modules except .init.text
> > to be patched in module_finalize().
> >
> > For that we use mod->core_layout.base/mod->core_layout.text_size as the
>
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
>
> Bottom line is: personal pronouns are ambiguous in text, especially with
> so many parties/companies/etc developing the kernel so let's avoid them
> please.
>

Thanks for your time looking at this. I'll reword the commit description
and submit a v2 shortly.

Regards,

--
Julian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]