Messages in this thread |  | | Date | Sun, 13 Mar 2022 21:05:35 +0300 | From | Alexey Dobriyan <> | Subject | Re: [PATCH 3/5] x86/alternative: record .altinstructions section entity size |
| |
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.
|  |