Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 13 Mar 2022 21:05:35 +0300
From:   Alexey Dobriyan <adobriyan@gmail.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/5] x86/alternative: record .altinstructions section
 entity size
Message-ID: <Yi4ybwog/H4gk5Ts@localhost.localdomain>
References: <20220311144312.88466-1-adobriyan@gmail.com>
 <20220311144312.88466-3-adobriyan@gmail.com>
 <20220312211740.GG28057@worktop.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220312211740.GG28057@worktop.programming.kicks-ass.net>
Precedence: bulk

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.