Message-ID: <2ac89a5fc103f895a185010f11c81014dcb58d9b.camel@intel.com>
Date: Wed, 8 Oct 2025 16:22:40 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Mehta, Sohil" <sohil.mehta@...el.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "bp@...en8.de"
	<bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>
CC: "corbet@....net" <corbet@....net>, "ardb@...nel.org" <ardb@...nel.org>,
	"andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
	"alexander.shishkin@...ux.intel.com" <alexander.shishkin@...ux.intel.com>,
	"luto@...nel.org" <luto@...nel.org>, "david.laight.linux@...il.com"
	<david.laight.linux@...il.com>, "jpoimboe@...nel.org" <jpoimboe@...nel.org>,
	"Luck, Tony" <tony.luck@...el.com>, "linux-efi@...r.kernel.org"
	<linux-efi@...r.kernel.org>, "kas@...nel.org" <kas@...nel.org>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "dwmw@...zon.co.uk"
	<dwmw@...zon.co.uk>, "rdunlap@...radead.org" <rdunlap@...radead.org>,
	"vegard.nossum@...cle.com" <vegard.nossum@...cle.com>, "xin@...or.com"
	<xin@...or.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "kees@...nel.org" <kees@...nel.org>,
	"hpa@...or.com" <hpa@...or.com>, "peterz@...radead.org"
	<peterz@...radead.org>, "geert@...ux-m68k.org" <geert@...ux-m68k.org>
Subject: Re: [PATCH v10 03/15] x86/alternatives: Disable LASS when patching
 kernel alternatives

On Tue, 2025-10-07 at 15:28 -0700, Sohil Mehta wrote:
> On 10/7/2025 9:55 AM, Edgecombe, Rick P wrote:
> > It's not just used for alternatives anymore. bpf, kprobes, etc use it too. Maybe
> > drop "alternatives" from the subject?
> > 
> 
> Yeah, I was just being lazy. The file is still called alternatives.c and
> that's probably what most folks are familiar with.
> 
> How about:
> x86/text-patching: Disable LASS when patching kernel code

"x86/alternatives: Disable LASS when patching kernel alternatives"

I meant the last reference there ^. I think the "x86/alternatives:" part should
match the rest of the commits to the file.

"x86/alternatives: Disable LASS when patching kernel kernel code" sounds more
accurate to me.

> 
> > 
> > The above variant has "a barrier is implicit in alternative", is it not needed
> > here too? Actually, not sure what that comment is trying to convey anyway.
> > 
> 
> Yes, the same implication holds true for the LASS versions as well.
> I assume it is to let users know that a separate memory barrier is not
> needed to prevent the memory accesses following the STAC/CLAC
> instructions from getting reordered.
> 
> I will add a similar note to the lass_clac()/stac() comments as well.

Up to you.

> 
> > Not a strong opinion, but the naming of stac()/clac() lass_stac()/lass_clac() is
> > a bit confusing to me. stac/clac instructions now has LASS and SMAP behavior.
> > Why keep the smap behavior implicit and give LASS a special variant?
> > 
> > The other odd aspect is that calling stac()/clac() is needed for LASS in some
> > places too, right? But stac()/clac() depend on X86_FEATURE_SMAP not
> > X86_FEATURE_LASS. A reader might wonder, why do we not need the lass variant
> > there too.
> > 
> > I'd expect in the real world LASS won't be found without SMAP. Maybe it could be
> > worth just improving the comment around stac()/clac() to include some nod that
> > it is doing LASS stuff too, or that it relies on that USER mappings are only
> > found in the lower half, but KERNEL mappings are not only found upper half.
> > 
> 
> LASS (data access) depends on SMAP in the hardware as well as the
> kernel. The STAC/CLAC instructions toggle LASS alongwith SMAP. One
> option is to use the current stac()/clac() instruction for all cases.
> However, that would mean unnecessary AC bit toggling during
> text-patching on systems without LASS.

Honestly, just unconditionally doing stac/clac doesn't sound that bad to me. We
already unconditionally enable SMAP, right? If there was some big slowdown for a
single copy, people would want an option to disable it. And with text patching
it's part a heavier operation already.

Was there previous feedback on that option?

> 
> The code comments mainly describe how these helpers should be used,
> rather than why they exist the way they do.
> 
> > > /* Use stac()/clac() when accessing userspace (_PAGE_USER)
> > > mappings, regardless of location. */
> > > 
> > > /* Use lass_stac()/lass_clac() when accessing kernel mappings (!
> > > _PAGE_USER) in the lower half of the address space. */
> Does this look accurate? The difference is subtle. Also, there is some
> potential for incorrect usage, but Dave would prefer to track them
> separately.
> 
> I can add more explanation to the commit message. Any preferred wording?
> Also, the separate patch that Dave recommended would help clarify things
> as well.

No preference. The main comment was just that, as someone looking at this part
fresh, it was a little unclear. Especially with the non-obvious SMAP-LASS link.

> 
> > >  
> > > +/*
> > > + * Text poking creates and uses a mapping in the lower half of the
> > > + * address space. Relax LASS enforcement when accessing the poking
> > > + * address.
> > > + *
> > > + * Also, objtool enforces a strict policy of "no function calls within
> > > + * AC=1 regions". Adhere to the policy by using inline versions of
> > > + * memcpy()/memset() that will never result in a function call.
> > 
> > Is "Also, ..." here really a separate issue? What is the connection to
> > lass_stac/clac()?
> > 
> 
> The issues are interdependent. We need the STAC/CLAC because text poking
> accesses special memory. We require the inline memcpy/memset because we
> have now added the STAC/CLAC usage and objtool guards against the
> potential misuse of STAC/CLAC.
> 
> Were you looking for any specific change to the wording?

Ah ok, but the compiler could have always uninlined the existing memcpy calls
right? So there is an existing theoretical problem, I would think.

But that link sounds strong enough to do it in one patch. If it were me I would
nod at the existing theoretical issue.

> 
> > >  static void text_poke_memcpy(void *dst, const void *src, size_t len)
> > >  {
> > > -	memcpy(dst, src, len);
> > > +	lass_stac();
> > > +	__inline_memcpy(dst, src, len);
> > > +	lass_clac();
> > >  }
> > >  
> 
> 

Powered by blists - more mailing lists