.altinstructions entry was 12 bytes in size, then it was 13 bytes,
now it is 12 again. It was 24 bytes on some distros as well.
Record this information as section sh_entsize value so that tools
which parse .altinstructions have easier time.
Signed-off-by: Alexey Dobriyan (CloudLinux) <[email protected]>
---
arch/x86/include/asm/alternative.h | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 58eee6402832..cf7722a106b3 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -9,6 +9,8 @@
#define ALTINSTR_FLAG_INV (1 << 15)
#define ALT_NOT(feat) ((feat) | ALTINSTR_FLAG_INV)
+#define sizeof_struct_alt_instr 12
+
#ifndef __ASSEMBLY__
#include <linux/stddef.h>
@@ -66,6 +68,7 @@ struct alt_instr {
u8 instrlen; /* length of original instruction */
u8 replacementlen; /* length of new instruction */
} __packed;
+_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");
/*
* Debug flag that can be tested to see whether alternative
@@ -159,7 +162,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
/* alternative assembly primitive: */
#define ALTERNATIVE(oldinstr, newinstr, feature) \
OLDINSTR(oldinstr, 1) \
- ".pushsection .altinstructions,\"a\"\n" \
+ ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
ALTINSTR_ENTRY(feature, 1) \
".popsection\n" \
".pushsection .altinstr_replacement, \"ax\"\n" \
@@ -168,7 +171,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
#define ALTERNATIVE_2(oldinstr, newinstr1, feature1, newinstr2, feature2)\
OLDINSTR_2(oldinstr, 1, 2) \
- ".pushsection .altinstructions,\"a\"\n" \
+ ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
ALTINSTR_ENTRY(feature1, 1) \
ALTINSTR_ENTRY(feature2, 2) \
".popsection\n" \
@@ -184,7 +187,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
#define ALTERNATIVE_3(oldinsn, newinsn1, feat1, newinsn2, feat2, newinsn3, feat3) \
OLDINSTR_3(oldinsn, 1, 2, 3) \
- ".pushsection .altinstructions,\"a\"\n" \
+ ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
ALTINSTR_ENTRY(feat1, 1) \
ALTINSTR_ENTRY(feat2, 2) \
ALTINSTR_ENTRY(feat3, 3) \
@@ -331,7 +334,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
.skip -(((144f-143f)-(141b-140b)) > 0) * ((144f-143f)-(141b-140b)),0x90
142:
- .pushsection .altinstructions,"a"
+ .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
altinstruction_entry 140b,143f,\feature,142b-140b,144f-143f
.popsection
@@ -368,7 +371,7 @@ static inline int alternatives_text_reserved(void *start, void *end)
(alt_max_short(new_len1, new_len2) - (old_len)),0x90
142:
- .pushsection .altinstructions,"a"
+ .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
altinstruction_entry 140b,143f,\feature1,142b-140b,144f-143f
altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f
.popsection
--
2.34.1
On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
> .altinstructions entry was 12 bytes in size, then it was 13 bytes,
> now it is 12 again. It was 24 bytes on some distros as well.
> Record this information as section sh_entsize value so that tools
> which parse .altinstructions have easier time.
Which tools would that be? Because afaict you've not actually updated
objtool.
> Signed-off-by: Alexey Dobriyan (CloudLinux) <[email protected]>
> ---
> arch/x86/include/asm/alternative.h | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 58eee6402832..cf7722a106b3 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -9,6 +9,8 @@
> #define ALTINSTR_FLAG_INV (1 << 15)
> #define ALT_NOT(feat) ((feat) | ALTINSTR_FLAG_INV)
>
> +#define sizeof_struct_alt_instr 12
> +
> #ifndef __ASSEMBLY__
>
> #include <linux/stddef.h>
> @@ -66,6 +68,7 @@ struct alt_instr {
> u8 instrlen; /* length of original instruction */
> u8 replacementlen; /* length of new instruction */
> } __packed;
> +_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");
Would it not be much simpler to have this in asm-offsets.h ?
> + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
> + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
Aside of adding entsize, you're also adding the M(ergable) bit. Also,
those lines are on the unwieldy side of things.
On Sat, Mar 12, 2022 at 10:17:40PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
> > .altinstructions entry was 12 bytes in size, then it was 13 bytes,
> > now it is 12 again. It was 24 bytes on some distros as well.
> > Record this information as section sh_entsize value so that tools
> > which parse .altinstructions have easier time.
>
> Which tools would that be? Because afaict you've not actually updated
> objtool.
We parse .altinstructions to look for "dangerous" functions so that we
don't unpatch when a process is sleeping in a userspace pagefault caused
by such function. Defining .sh_entsize will simplify this process in the future.
Now that padding issues have been solved, "struct alt_instr" should be
stable and sizeof should be enough to tell one layout from another.
> > --- a/arch/x86/include/asm/alternative.h
> > +++ b/arch/x86/include/asm/alternative.h
> > @@ -9,6 +9,8 @@
> > #define ALTINSTR_FLAG_INV (1 << 15)
> > #define ALT_NOT(feat) ((feat) | ALTINSTR_FLAG_INV)
> >
> > +#define sizeof_struct_alt_instr 12
> > +
> > #ifndef __ASSEMBLY__
> >
> > #include <linux/stddef.h>
> > @@ -66,6 +68,7 @@ struct alt_instr {
> > u8 instrlen; /* length of original instruction */
> > u8 replacementlen; /* length of new instruction */
> > } __packed;
> > +_Static_assert(sizeof(struct alt_instr) == sizeof_struct_alt_instr, "");
>
> Would it not be much simpler to have this in asm-offsets.h ?
I tried this and failed. alternative.h is getting included and
preprocessed before asm-offsets.c is generated so there are lines like
#define 12 12
and it doesn't work.
> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>
> > + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
> > + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
>
> Aside of adding entsize, you're also adding the M(ergable) bit. Also,
> those lines are on the unwieldy side of things.
binutils doc says
https://sourceware.org/binutils/docs/as/Section.html
If flags contains the M symbol then the type argument must be specified as well as an extra argument—entsize—like this:
.section name , "flags"M, @type, entsize
Sections with the M flag but not S flag must contain fixed size constants,
each entsize octets long. Sections with both M and S must contain zero
terminated strings where each character is entsize bytes long. The linker
may remove duplicates within sections with the same name, same entity size
and same flags. entsize must be an absolute expression. For sections with
both M and S, a string which is a suffix of a larger string is considered
a duplicate. Thus "def" will be merged with "abcdef"; A reference to the
first "def" will be changed to a reference to "abcdef"+3.
"a"M doesn't work, but "aM" does.
I don't know if merging is the issue, it is not like alt replacements have names.
On Sun, Mar 13 2022 at 21:05, Alexey Dobriyan wrote:
> On Sat, Mar 12, 2022 at 10:17:40PM +0100, Peter Zijlstra wrote:
>> On Fri, Mar 11, 2022 at 05:43:10PM +0300, Alexey Dobriyan wrote:
>> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>> > + ".pushsection .altinstructions,\"aM\",@progbits," __stringify(sizeof_struct_alt_instr) "\n"\
>>
>> > + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
>> > + .pushsection .altinstructions,"aM",@progbits,sizeof_struct_alt_instr
>>
>> Aside of adding entsize, you're also adding the M(ergable) bit. Also,
>> those lines are on the unwieldy side of things.
>
> binutils doc says
>
> https://sourceware.org/binutils/docs/as/Section.html
>
> If flags contains the M symbol then the type argument must be specified as well as an extra argument—entsize—like this:
>
> .section name , "flags"M, @type, entsize
>
> Sections with the M flag but not S flag must contain fixed size constants,
> each entsize octets long. Sections with both M and S must contain zero
> terminated strings where each character is entsize bytes long. The linker
> may remove duplicates within sections with the same name, same entity size
> and same flags. entsize must be an absolute expression. For sections with
> both M and S, a string which is a suffix of a larger string is considered
> a duplicate. Thus "def" will be merged with "abcdef"; A reference to the
> first "def" will be changed to a reference to "abcdef"+3.
>
> "a"M doesn't work, but "aM" does.
>
> I don't know if merging is the issue, it is not like alt replacements have names.
That does not matter. M merges any duplications in sections with the
same [section] name, entity size and flags.
.pushsection .bar "aM" @progbits, 4
.byte 0x01, 0x02, 0x03, 0x04
.popsection
.pushsection .bar "aM" @progbits, 4
.byte 0x01, 0x02, 0x03, 0x04
.popsection
Will create a section .bar with lenght 4 and the content:
0x1,0x2,0x3,0x4
What saves you here is the fact that the altinstruction entries are
guaranteed to be unique, but that wants a big fat comment.
Thanks,
tglx
On 05/04/2022 21.24, Thomas Gleixner wrote:
> On Sun, Mar 13 2022 at 21:05, Alexey Dobriyan wrote:
> That does not matter. M merges any duplications in sections with the
> same [section] name, entity size and flags.
>
> .pushsection .bar "aM" @progbits, 4
> .byte 0x01, 0x02, 0x03, 0x04
> .popsection
>
> .pushsection .bar "aM" @progbits, 4
> .byte 0x01, 0x02, 0x03, 0x04
> .popsection
>
> Will create a section .bar with lenght 4 and the content:
> 0x1,0x2,0x3,0x4
>
> What saves you here is the fact that the altinstruction entries are
> guaranteed to be unique, but that wants a big fat comment.
Actually, I think what saves this is that the linker at least currently
ignores the merge flag for sections with relocations; from binutils
bfd/merge.c:
if ((sec->flags & SEC_RELOC) != 0)
{
/* We aren't prepared to handle relocations in merged sections. */
return true;
}
I do think it is theoretically possible for two altinstruction entries
to end up being identical after relocations have been applied (same
relative offsets to both the .text section and their replacement
instructions).
Rasmus