Re: [PATCHv8 03/17] x86/alternatives: Disable LASS when patching kernel alternatives

Re: [PATCHv8 03/17] x86/alternatives: Disable LASS when patching kernel alternatives

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
>  
> +/*
> + * The CLAC/STAC instructions toggle the enforcement of X86_FEATURE_SMAP and
> + * X86_FEATURE_LASS.
> + *
> + * SMAP enforcement is based on the _PAGE_BIT_USER bit in the page tables: the
> + * kernel is not allowed to touch pages with the bit set unless the AC bit is
> + * set.
> + *
> + * LASS enforcement is based on bit 63 of the virtual address. The kernel is
> + * not allowed to touch memory in the lower half of the virtual address space
> + * unless the AC bit is set.
> + *
> + * 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.
> + *
> + * Note: a barrier is implicit in alternative().
> + */
> +
Thank you for incorporating my feedback. I like the updated wording.

Re: [PATCHv8 04/17] x86/cpu: Defer CR pinning setup until after EFI initialization

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> 
> In order to map the EFI runtime services, set_virtual_address_map()
> needs to be called, which resides in the lower half of the address
> space. This means that LASS needs to be temporarily disabled around
> this call. This can only be done before the CR pinning is set up.
> 
> Move CR pinning setup behind the EFI initialization.
> 
> Wrapping efi_enter_virtual_mode() into lass_disable/enable_enforcement()

I believe this should be lass_stac()/clac() since we reverted to the
original naming.

> is not enough because AC flag gates data accesses, but not instruction
> fetch. Clearing the CR4 bit is required.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/common.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 4f430be285de..9918121e0adc 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2081,7 +2081,6 @@ static __init void identify_boot_cpu(void)
>  	enable_sep_cpu();
>  #endif
>  	cpu_detect_tlb(&boot_cpu_data);
> -	setup_cr_pinning();
>  
>  	tsx_init();
>  	tdx_init();
> @@ -2532,10 +2531,14 @@ void __init arch_cpu_finalize_init(void)
>  
>  	/*
>  	 * This needs to follow the FPU initializtion, since EFI depends on it.
> +	 *
> +	 * EFI twiddles CR4.LASS. Do it before CR pinning.
>  	 */
>  	if (efi_enabled(EFI_RUNTIME_SERVICES))
>  		efi_enter_virtual_mode();
>  
> +	setup_cr_pinning();
> +

Instead of EFI toggling CR4.LASS, why not defer the first LASS
activation itself?

i.e.

	if (efi_enabled(EFI_RUNTIME_SERVICES))
		efi_enter_virtual_mode();

	setup_lass();

	setup_cr_pinning();


This way, we can avoid the following patch (#5) altogether.

Re: [PATCHv8 04/17] x86/cpu: Defer CR pinning setup until after EFI initialization

[Kirill A. Shutemov]

On Tue, Jul 01, 2025 at 12:03:01PM -0700, Sohil Mehta wrote:
> On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> > From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > 
> > In order to map the EFI runtime services, set_virtual_address_map()
> > needs to be called, which resides in the lower half of the address
> > space. This means that LASS needs to be temporarily disabled around
> > this call. This can only be done before the CR pinning is set up.
> > 
> > Move CR pinning setup behind the EFI initialization.
> > 
> > Wrapping efi_enter_virtual_mode() into lass_disable/enable_enforcement()
> 
> I believe this should be lass_stac()/clac() since we reverted to the
> original naming.

Doh. Will fix.

> > is not enough because AC flag gates data accesses, but not instruction
> > fetch. Clearing the CR4 bit is required.
> > 
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/common.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 4f430be285de..9918121e0adc 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -2081,7 +2081,6 @@ static __init void identify_boot_cpu(void)
> >  	enable_sep_cpu();
> >  #endif
> >  	cpu_detect_tlb(&boot_cpu_data);
> > -	setup_cr_pinning();
> >  
> >  	tsx_init();
> >  	tdx_init();
> > @@ -2532,10 +2531,14 @@ void __init arch_cpu_finalize_init(void)
> >  
> >  	/*
> >  	 * This needs to follow the FPU initializtion, since EFI depends on it.
> > +	 *
> > +	 * EFI twiddles CR4.LASS. Do it before CR pinning.
> >  	 */
> >  	if (efi_enabled(EFI_RUNTIME_SERVICES))
> >  		efi_enter_virtual_mode();
> >  
> > +	setup_cr_pinning();
> > +
> 
> Instead of EFI toggling CR4.LASS, why not defer the first LASS
> activation itself?
> 
> i.e.
> 
> 	if (efi_enabled(EFI_RUNTIME_SERVICES))
> 		efi_enter_virtual_mode();
> 
> 	setup_lass();
> 
> 	setup_cr_pinning();
> 
> 
> This way, we can avoid the following patch (#5) altogether.

That's definitely an option.

The benefit of current approach is that the enforcement is enabled
earlier and cover more boot code, providing marginal protection
improvement.

I also like that related security features (SMEP/SMAP/UMIP/LASS) are
enabled in the same place.

In the end it is a judgement call.

Maintainers, any preference?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 04/17] x86/cpu: Defer CR pinning setup until after EFI initialization

[Dave Hansen]

On 7/1/25 02:58, Kirill A. Shutemov wrote:
> Move CR pinning setup behind the EFI initialization.

I kinda grumble about these one-off solutions. Could we just do this
once and for all and defer CR pinning as long as possible? For instance,
could we do it in a late_initcall()?

Do we need pinning before userspace comes up?

Re: [PATCHv8 04/17] x86/cpu: Defer CR pinning setup until after EFI initialization

[Kirill A. Shutemov]

On Tue, Jul 01, 2025 at 04:10:19PM -0700, Dave Hansen wrote:
> On 7/1/25 02:58, Kirill A. Shutemov wrote:
> > Move CR pinning setup behind the EFI initialization.
> 
> I kinda grumble about these one-off solutions. Could we just do this
> once and for all and defer CR pinning as long as possible? For instance,
> could we do it in a late_initcall()?
> 
> Do we need pinning before userspace comes up?

Hm. I operated from an assumption that we want to pin control registers as
early as possible to get most benefit from it.

I guess we can defer it until later. But I am not sure late_initcall() is
the right place. Do we want random driver to twiddle control registers?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 04/17] x86/cpu: Defer CR pinning setup until after EFI initialization

[Kirill A. Shutemov]

On Wed, Jul 02, 2025 at 01:05:23PM +0300, Kirill A. Shutemov wrote:
> On Tue, Jul 01, 2025 at 04:10:19PM -0700, Dave Hansen wrote:
> > On 7/1/25 02:58, Kirill A. Shutemov wrote:
> > > Move CR pinning setup behind the EFI initialization.
> > 
> > I kinda grumble about these one-off solutions. Could we just do this
> > once and for all and defer CR pinning as long as possible? For instance,
> > could we do it in a late_initcall()?
> > 
> > Do we need pinning before userspace comes up?
> 
> Hm. I operated from an assumption that we want to pin control registers as
> early as possible to get most benefit from it.
> 
> I guess we can defer it until later. But I am not sure late_initcall() is
> the right place. Do we want random driver to twiddle control registers?

I will do it in core_initcall().

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 11/17] x86/cpu: Set LASS CR4 bit as pinning sensitive

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> From: Yian Chen <yian.chen@intel.com>
> 
> Security features such as LASS are not expected to be disabled once
> initialized. Add LASS to the CR4 pinned mask.
> 
> Signed-off-by: Yian Chen <yian.chen@intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

I think this CR4 pinning change can be merged with the other CR pinning
related patch (#4). At a minimum, this should be placed close to that
patch to make logical sense.

1) Add LASS to the CR4 pinned mask
2) Defer CR pinning since it would cause XYZ issue.

Or the other way around. Anyway,

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 9918121e0adc..1552c7510380 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -403,7 +403,8 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
>  
>  /* These bits should not change their value after CPU init is finished. */
>  static const unsigned long cr4_pinned_mask = X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
> -					     X86_CR4_FSGSBASE | X86_CR4_CET | X86_CR4_FRED;
> +					     X86_CR4_FSGSBASE | X86_CR4_CET | X86_CR4_FRED |
> +					     X86_CR4_LASS;
>  static DEFINE_STATIC_KEY_FALSE_RO(cr_pinning);
>  static unsigned long cr4_pinned_bits __ro_after_init;
>  

Re: [PATCHv8 15/17] x86/cpu: Make LAM depend on LASS

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> 
> To prevent exploits for Spectre based on LAM as demonstrated by the
> whitepaper [1], make LAM depend on LASS, which avoids this type of
> vulnerability.
> 
> [1] https://download.vusec.net/papers/slam_sp24.pdf
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/cpuid-deps.c | 1 +
>  1 file changed, 1 insertion(+)
> 

In terms of patch ordering, wouldn't it make more sense to introduce LAM
related changes after LASS has been fully enabled? This patch should
probably be after Patch #16 which enables LASS.

Logically, the LAM re-enabling stuff can be a separate series, but since
it's only a few changed lines having it at the end seems okay.

Patch 1-15  => Enable LASS
Patch 16-17 => Re-enable LAM

Other than that,

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 98d0cdd82574..11bb9ed40140 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -90,6 +90,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>  	{ X86_FEATURE_FRED,			X86_FEATURE_LKGS      },
>  	{ X86_FEATURE_SPEC_CTRL_SSBD,		X86_FEATURE_SPEC_CTRL },
>  	{ X86_FEATURE_LASS,			X86_FEATURE_SMAP      },
> +	{ X86_FEATURE_LAM,			X86_FEATURE_LASS      },
>  	{}
>  };
>  

Re: [PATCHv8 17/17] x86: Re-enable Linear Address Masking

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> This reverts commit 3267cb6d3a174ff83d6287dcd5b0047bbd912452.
> 

This patch isn't truly a revert. This line can be skipped since the
additional changes going in are more than the reverted single line.

> LASS mitigates the Spectre based on LAM (SLAM) [1] and the previous
> commit made LAM depend on LASS, so we no longer need to disable LAM at
> compile time, so revert the commit that disables LAM.

Also, wording such as previous commit should be avoided since it can be
misleading. For example, in this series "the previous commit" is
enabling LASS. The commit before that adds the actual dependency between
LAM and LASS.

Other than that, the code changes look good to me.

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

> 
> Adjust USER_PTR_MAX if LAM enabled, allowing tag bits to be set for
> userspace pointers. The value for the constant is defined in a way to
> avoid overflow compiler warning on 32-bit config.
> 
> [1] https://download.vusec.net/papers/slam_sp24.pdf
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  arch/x86/Kconfig             | 1 -
>  arch/x86/kernel/cpu/common.c | 5 +----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 71019b3b54ea..2b48e916b754 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2181,7 +2181,6 @@ config RANDOMIZE_MEMORY_PHYSICAL_PADDING
>  config ADDRESS_MASKING
>  	bool "Linear Address Masking support"
>  	depends on X86_64
> -	depends on COMPILE_TEST || !CPU_MITIGATIONS # wait for LASS
>  	help
>  	  Linear Address Masking (LAM) modifies the checking that is applied
>  	  to 64-bit linear addresses, allowing software to use of the
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 97a228f917a9..6f2ae9e702bc 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2558,11 +2558,8 @@ void __init arch_cpu_finalize_init(void)
>  	if (IS_ENABLED(CONFIG_X86_64)) {
>  		unsigned long USER_PTR_MAX = TASK_SIZE_MAX;
>  
> -		/*
> -		 * Enable this when LAM is gated on LASS support
>  		if (cpu_feature_enabled(X86_FEATURE_LAM))
> -			USER_PTR_MAX = (1ul << 63) - PAGE_SIZE;
> -		 */
> +			USER_PTR_MAX = (-1UL >> 1) & PAGE_MASK;
>  		runtime_const_init(ptr, USER_PTR_MAX);
>  
>  		/*

Re: [PATCHv8 12/17] x86/traps: Communicate a LASS violation in #GP message

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
>  /*
> @@ -672,6 +681,12 @@ static enum kernel_gp_hint get_kernel_gp_address(struct pt_regs *regs,
>  	if (*addr < ~__VIRTUAL_MASK &&
>  	    *addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
>  		return GP_NON_CANONICAL;
> +	else if (*addr < ~__VIRTUAL_MASK &&
> +		 cpu_feature_enabled(X86_FEATURE_LASS)) {
> +		if (*addr < PAGE_SIZE)
> +			return GP_NULL_POINTER;
> +		return GP_LASS_VIOLATION;
> +	}

The comments above this section of code say:

/*
 * Check that:
 *  - the operand is not in the kernel half
 *  - the last byte of the operand is not in the user canonical half
 */

They should be updated since we are updating the logic.

Also, below is easier to read than above:

	if (*addr < ~__VIRTUAL_MASK) {

		if (*addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
			return EXC_NON_CANONICAL;

		if (cpu_feature_enabled(X86_FEATURE_LASS)) {
			if (*addr < PAGE_SIZE)
				return EXC_NULL_POINTER;
			return EXC_LASS_VIOLATION;
		}
	}

I am wondering if the NULL pointer exception should be made
unconditional, even if it is unlikely to reach here without LASS. So
maybe something like this:

	if (*addr < ~__VIRTUAL_MASK) {

		if (*addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
			return EXC_NON_CANONICAL;

		if (*addr < PAGE_SIZE)
			return EXC_NULL_POINTER;

		if (cpu_feature_enabled(X86_FEATURE_LASS))
			return EXC_LASS_VIOLATION;
	}

Re: [PATCHv8 12/17] x86/traps: Communicate a LASS violation in #GP message

[Kirill A. Shutemov]

On Tue, Jul 01, 2025 at 05:36:06PM -0700, Sohil Mehta wrote:
> On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> >  /*
> > @@ -672,6 +681,12 @@ static enum kernel_gp_hint get_kernel_gp_address(struct pt_regs *regs,
> >  	if (*addr < ~__VIRTUAL_MASK &&
> >  	    *addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
> >  		return GP_NON_CANONICAL;
> > +	else if (*addr < ~__VIRTUAL_MASK &&
> > +		 cpu_feature_enabled(X86_FEATURE_LASS)) {
> > +		if (*addr < PAGE_SIZE)
> > +			return GP_NULL_POINTER;
> > +		return GP_LASS_VIOLATION;
> > +	}
> 
> The comments above this section of code say:
> 
> /*
>  * Check that:
>  *  - the operand is not in the kernel half
>  *  - the last byte of the operand is not in the user canonical half
>  */
> 
> They should be updated since we are updating the logic.

Okay.

> Also, below is easier to read than above:
> 
> 	if (*addr < ~__VIRTUAL_MASK) {
> 
> 		if (*addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
> 			return EXC_NON_CANONICAL;
> 
> 		if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> 			if (*addr < PAGE_SIZE)
> 				return EXC_NULL_POINTER;
> 			return EXC_LASS_VIOLATION;
> 		}
> 	}
> 
> I am wondering if the NULL pointer exception should be made
> unconditional, even if it is unlikely to reach here without LASS. So
> maybe something like this:
> 
> 	if (*addr < ~__VIRTUAL_MASK) {
> 
> 		if (*addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK)
> 			return EXC_NON_CANONICAL;
> 
> 		if (*addr < PAGE_SIZE)
> 			return EXC_NULL_POINTER;
> 
> 		if (cpu_feature_enabled(X86_FEATURE_LASS))
> 			return EXC_LASS_VIOLATION;
> 	}

That's cleaner.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 13/17] x86/traps: Generalize #GP address decode and hint code

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> Handlers for #GP and #SS will now share code to decode the exception
> address and retrieve the exception hint string.
> 

This is missing an essential "why"? Why do #GP and #SS handlers need to
share code? None of the patches prior to this have hinted this.

It can probably be deduced from a later patch, but it needs to be
clarified in this one. Maybe a simplified version of the text from the SDM:

"In most cases, an access causing a LASS violation results in a general
protection exception (#GP); for stack accesses (those due to
stack-oriented instructions, as well as accesses that implicitly or
explicitly use the SS segment register), a stack fault (#SS) is generated."

> The helper, enum, and array should be renamed as they are no longer
> specific to #GP.
> 
> No functional change intended.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/traps.c | 62 ++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 

The code changes look okay to me except a minor nit below.

>  #define GPFSTR "general protection fault"
> @@ -808,8 +808,8 @@ static void gp_user_force_sig_segv(struct pt_regs *regs, int trapnr,
>  DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  {
>  	char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
> -	enum kernel_gp_hint hint = GP_NO_HINT;
> -	unsigned long gp_addr;

gp_addr is a local variable to the #GP handler. It can probably stay the
same.

> +	enum kernel_exc_hint hint = EXC_NO_HINT;
> +	unsigned long exc_addr;
>  
>  	if (user_mode(regs) && try_fixup_enqcmd_gp())
>  		return;
> @@ -846,21 +846,21 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  	if (error_code)
>  		snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
>  	else
> -		hint = get_kernel_gp_address(regs, &gp_addr);
> +		hint = get_kernel_exc_address(regs, &exc_addr);
>  
> -	if (hint != GP_NO_HINT) {
> +	if (hint != EXC_NO_HINT) {
>  		snprintf(desc, sizeof(desc), GPFSTR ", %s 0x%lx",
> -			 kernel_gp_hint_help[hint], gp_addr);
> +			 kernel_exc_hint_help[hint], exc_addr);
>  	}
>  
>  	/*
>  	 * KASAN is interested only in the non-canonical case, clear it
>  	 * otherwise.
>  	 */
> -	if (hint != GP_NON_CANONICAL)
> -		gp_addr = 0;
> +	if (hint != EXC_NON_CANONICAL)
> +		exc_addr = 0;
>  
> -	die_addr(desc, regs, error_code, gp_addr);
> +	die_addr(desc, regs, error_code, exc_addr);
>  
>  exit:
>  	cond_local_irq_disable(regs);

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Sohil Mehta]

On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> LASS throws a #GP for any violations except for stack register accesses,
> in which case it throws a #SS instead. Handle this similarly to how other
> LASS violations are handled.
> 

Maybe I've misunderstood something:

Is the underlying assumption here that #SS were previously only
generated by userspace, but now they can also be generated by the
kernel? And we want the kernel generated #SS to behave the same as the #GP?

> In case of FRED, before handling #SS as LASS violation, kernel has to
> check if there's a fixup for the exception. It can address #SS due to
> invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
> fault on ERETU by jumping to fred_entrypoint_user") for more details.
> 
> Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index ceb091f17a5b..f9ca5b911141 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
>  		      SIGBUS, 0, NULL);
>  }
>  
> -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> -{
> -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> -		      0, NULL);
> -}
> -
>  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>  {
>  	char *str = "alignment check";
> @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>  	cond_local_irq_disable(regs);
>  }
>  
> +#define SSFSTR "stack segment fault"
> +
> +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> +{
> +	if (user_mode(regs))
> +		goto error_trap;
> +
> +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
> +		return;
> +
> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> +		enum kernel_exc_hint hint;
> +		unsigned long exc_addr;
> +
> +		hint = get_kernel_exc_address(regs, &exc_addr);
> +		if (hint != EXC_NO_HINT) {

The brackets are not needed for singular statements. Also the max line
length is longer now. You can fit this all in a single line.

> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
> +			       exc_addr);
> +		}
> +

> +		if (hint != EXC_NON_CANONICAL)
> +			exc_addr = 0;
> +
> +		die_addr(SSFSTR, regs, error_code, exc_addr);

The variable names in die_addr() should be generalized as well. They
seem to assume the caller to be a #GP handler.

> +		return;
> +	}
> +
> +error_trap:
> +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> +		      0, NULL);
> +}
> +
>  static bool do_int3(struct pt_regs *regs)
>  {
>  	int res;

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 1, 2025 6:35:40 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
>On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
>> LASS throws a #GP for any violations except for stack register accesses,
>> in which case it throws a #SS instead. Handle this similarly to how other
>> LASS violations are handled.
>> 
>
>Maybe I've misunderstood something:
>
>Is the underlying assumption here that #SS were previously only
>generated by userspace, but now they can also be generated by the
>kernel? And we want the kernel generated #SS to behave the same as the #GP?
>
>> In case of FRED, before handling #SS as LASS violation, kernel has to
>> check if there's a fixup for the exception. It can address #SS due to
>> invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
>> fault on ERETU by jumping to fred_entrypoint_user") for more details.
>> 
>> Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> ---
>>  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
>>  1 file changed, 33 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index ceb091f17a5b..f9ca5b911141 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
>>  		      SIGBUS, 0, NULL);
>>  }
>>  
>> -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> -{
>> -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> -		      0, NULL);
>> -}
>> -
>>  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>>  {
>>  	char *str = "alignment check";
>> @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>>  	cond_local_irq_disable(regs);
>>  }
>>  
>> +#define SSFSTR "stack segment fault"
>> +
>> +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> +{
>> +	if (user_mode(regs))
>> +		goto error_trap;
>> +
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
>> +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
>> +		return;
>> +
>> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>> +		enum kernel_exc_hint hint;
>> +		unsigned long exc_addr;
>> +
>> +		hint = get_kernel_exc_address(regs, &exc_addr);
>> +		if (hint != EXC_NO_HINT) {
>
>The brackets are not needed for singular statements. Also the max line
>length is longer now. You can fit this all in a single line.
>
>> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
>> +			       exc_addr);
>> +		}
>> +
>
>> +		if (hint != EXC_NON_CANONICAL)
>> +			exc_addr = 0;
>> +
>> +		die_addr(SSFSTR, regs, error_code, exc_addr);
>
>The variable names in die_addr() should be generalized as well. They
>seem to assume the caller to be a #GP handler.
>
>> +		return;
>> +	}
>> +
>> +error_trap:
>> +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> +		      0, NULL);
>> +}
>> +
>>  static bool do_int3(struct pt_regs *regs)
>>  {
>>  	int res;
>

An #SS can be generated by the kernel if RSP is corrupted. This is fatal, but as always we want to get a message out.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 1, 2025 6:35:40 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
>On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
>> LASS throws a #GP for any violations except for stack register accesses,
>> in which case it throws a #SS instead. Handle this similarly to how other
>> LASS violations are handled.
>> 
>
>Maybe I've misunderstood something:
>
>Is the underlying assumption here that #SS were previously only
>generated by userspace, but now they can also be generated by the
>kernel? And we want the kernel generated #SS to behave the same as the #GP?
>
>> In case of FRED, before handling #SS as LASS violation, kernel has to
>> check if there's a fixup for the exception. It can address #SS due to
>> invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
>> fault on ERETU by jumping to fred_entrypoint_user") for more details.
>> 
>> Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> ---
>>  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
>>  1 file changed, 33 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> index ceb091f17a5b..f9ca5b911141 100644
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
>>  		      SIGBUS, 0, NULL);
>>  }
>>  
>> -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> -{
>> -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> -		      0, NULL);
>> -}
>> -
>>  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>>  {
>>  	char *str = "alignment check";
>> @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>>  	cond_local_irq_disable(regs);
>>  }
>>  
>> +#define SSFSTR "stack segment fault"
>> +
>> +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> +{
>> +	if (user_mode(regs))
>> +		goto error_trap;
>> +
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
>> +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
>> +		return;
>> +
>> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>> +		enum kernel_exc_hint hint;
>> +		unsigned long exc_addr;
>> +
>> +		hint = get_kernel_exc_address(regs, &exc_addr);
>> +		if (hint != EXC_NO_HINT) {
>
>The brackets are not needed for singular statements. Also the max line
>length is longer now. You can fit this all in a single line.
>
>> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
>> +			       exc_addr);
>> +		}
>> +
>
>> +		if (hint != EXC_NON_CANONICAL)
>> +			exc_addr = 0;
>> +
>> +		die_addr(SSFSTR, regs, error_code, exc_addr);
>
>The variable names in die_addr() should be generalized as well. They
>seem to assume the caller to be a #GP handler.
>
>> +		return;
>> +	}
>> +
>> +error_trap:
>> +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> +		      0, NULL);
>> +}
>> +
>>  static bool do_int3(struct pt_regs *regs)
>>  {
>>  	int res;
>

Note: for a FRED system, ERETU can generate #SS for a non-canonical user space RSP even in the absence of LASS, so if that is not currently handled that is an active bug.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Tue, Jul 01, 2025 at 07:06:10PM -0700, H. Peter Anvin wrote:
> On July 1, 2025 6:35:40 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
> >On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> >> LASS throws a #GP for any violations except for stack register accesses,
> >> in which case it throws a #SS instead. Handle this similarly to how other
> >> LASS violations are handled.
> >> 
> >
> >Maybe I've misunderstood something:
> >
> >Is the underlying assumption here that #SS were previously only
> >generated by userspace, but now they can also be generated by the
> >kernel? And we want the kernel generated #SS to behave the same as the #GP?
> >
> >> In case of FRED, before handling #SS as LASS violation, kernel has to
> >> check if there's a fixup for the exception. It can address #SS due to
> >> invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
> >> fault on ERETU by jumping to fred_entrypoint_user") for more details.
> >> 
> >> Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >> ---
> >>  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
> >>  1 file changed, 33 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> >> index ceb091f17a5b..f9ca5b911141 100644
> >> --- a/arch/x86/kernel/traps.c
> >> +++ b/arch/x86/kernel/traps.c
> >> @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
> >>  		      SIGBUS, 0, NULL);
> >>  }
> >>  
> >> -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> >> -{
> >> -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> >> -		      0, NULL);
> >> -}
> >> -
> >>  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
> >>  {
> >>  	char *str = "alignment check";
> >> @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> >>  	cond_local_irq_disable(regs);
> >>  }
> >>  
> >> +#define SSFSTR "stack segment fault"
> >> +
> >> +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> >> +{
> >> +	if (user_mode(regs))
> >> +		goto error_trap;
> >> +
> >> +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> >> +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
> >> +		return;
> >> +
> >> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> >> +		enum kernel_exc_hint hint;
> >> +		unsigned long exc_addr;
> >> +
> >> +		hint = get_kernel_exc_address(regs, &exc_addr);
> >> +		if (hint != EXC_NO_HINT) {
> >
> >The brackets are not needed for singular statements. Also the max line
> >length is longer now. You can fit this all in a single line.
> >
> >> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
> >> +			       exc_addr);
> >> +		}
> >> +
> >
> >> +		if (hint != EXC_NON_CANONICAL)
> >> +			exc_addr = 0;
> >> +
> >> +		die_addr(SSFSTR, regs, error_code, exc_addr);
> >
> >The variable names in die_addr() should be generalized as well. They
> >seem to assume the caller to be a #GP handler.
> >
> >> +		return;
> >> +	}
> >> +
> >> +error_trap:
> >> +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> >> +		      0, NULL);
> >> +}
> >> +
> >>  static bool do_int3(struct pt_regs *regs)
> >>  {
> >>  	int res;
> >
> 
> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space RSP even in the absence of LASS, so if that is not currently handled that is an active bug.

It is handled by fixup code inside do_error_trap(). We need to add
explicit fixup before LASS handling to avoid treating bad userspace RSP as
kernel LASS violation.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 2, 2025 3:17:10 AM PDT, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
>On Tue, Jul 01, 2025 at 07:06:10PM -0700, H. Peter Anvin wrote:
>> On July 1, 2025 6:35:40 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
>> >On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
>> >> LASS throws a #GP for any violations except for stack register accesses,
>> >> in which case it throws a #SS instead. Handle this similarly to how other
>> >> LASS violations are handled.
>> >> 
>> >
>> >Maybe I've misunderstood something:
>> >
>> >Is the underlying assumption here that #SS were previously only
>> >generated by userspace, but now they can also be generated by the
>> >kernel? And we want the kernel generated #SS to behave the same as the #GP?
>> >
>> >> In case of FRED, before handling #SS as LASS violation, kernel has to
>> >> check if there's a fixup for the exception. It can address #SS due to
>> >> invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
>> >> fault on ERETU by jumping to fred_entrypoint_user") for more details.
>> >> 
>> >> Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> >> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> >> ---
>> >>  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
>> >>  1 file changed, 33 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
>> >> index ceb091f17a5b..f9ca5b911141 100644
>> >> --- a/arch/x86/kernel/traps.c
>> >> +++ b/arch/x86/kernel/traps.c
>> >> @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
>> >>  		      SIGBUS, 0, NULL);
>> >>  }
>> >>  
>> >> -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> >> -{
>> >> -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> >> -		      0, NULL);
>> >> -}
>> >> -
>> >>  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
>> >>  {
>> >>  	char *str = "alignment check";
>> >> @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
>> >>  	cond_local_irq_disable(regs);
>> >>  }
>> >>  
>> >> +#define SSFSTR "stack segment fault"
>> >> +
>> >> +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
>> >> +{
>> >> +	if (user_mode(regs))
>> >> +		goto error_trap;
>> >> +
>> >> +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
>> >> +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
>> >> +		return;
>> >> +
>> >> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>> >> +		enum kernel_exc_hint hint;
>> >> +		unsigned long exc_addr;
>> >> +
>> >> +		hint = get_kernel_exc_address(regs, &exc_addr);
>> >> +		if (hint != EXC_NO_HINT) {
>> >
>> >The brackets are not needed for singular statements. Also the max line
>> >length is longer now. You can fit this all in a single line.
>> >
>> >> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
>> >> +			       exc_addr);
>> >> +		}
>> >> +
>> >
>> >> +		if (hint != EXC_NON_CANONICAL)
>> >> +			exc_addr = 0;
>> >> +
>> >> +		die_addr(SSFSTR, regs, error_code, exc_addr);
>> >
>> >The variable names in die_addr() should be generalized as well. They
>> >seem to assume the caller to be a #GP handler.
>> >
>> >> +		return;
>> >> +	}
>> >> +
>> >> +error_trap:
>> >> +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
>> >> +		      0, NULL);
>> >> +}
>> >> +
>> >>  static bool do_int3(struct pt_regs *regs)
>> >>  {
>> >>  	int res;
>> >
>> 
>> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space RSP even in the absence of LASS, so if that is not currently handled that is an active bug.
>
>It is handled by fixup code inside do_error_trap(). We need to add
>explicit fixup before LASS handling to avoid treating bad userspace RSP as
>kernel LASS violation.
>

Great. I was pretty sure, but I wanted to address Sohil's question directly. Thanks for verifying.

A LASS violation of any kind in the kernel (unless handled by fixup, including user access fixup) ought to be fatal, correct?

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Wed, Jul 02, 2025 at 07:37:12AM -0700, H. Peter Anvin wrote:
> A LASS violation of any kind in the kernel (unless handled by fixup,
> including user access fixup) ought to be fatal, correct?

Yes, LASS violation is fatal for !user_mode(regs), unless addressed by
fixups.

For user_mode(regs), emulate_vsyscall_gp() is the notable exception.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 2, 2025 7:47:30 AM PDT, "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
>On Wed, Jul 02, 2025 at 07:37:12AM -0700, H. Peter Anvin wrote:
>> A LASS violation of any kind in the kernel (unless handled by fixup,
>> including user access fixup) ought to be fatal, correct?
>
>Yes, LASS violation is fatal for !user_mode(regs), unless addressed by
>fixups.
>
>For user_mode(regs), emulate_vsyscall_gp() is the notable exception.
>

Note also that for FRED we can have separate kernel and user space paths basically "for free". I'm not sure if we do that yet (if the infrastructure is there), but we could. 

Not that it matters for this case. This is a slow path.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Andrew Cooper]

> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space RSP

How?  Or to phrase it differently, I hope not.

%rsp is a 64bit value and does not have canonical restrictions elsewhere
in the architecture, so far as I'm aware.  IRET really can restore a
non-canonical %rsp, and userspace can run for an indeterminate period of
time with a non-canonical %rsp as long as there are no stack accesses.

Accesses relative to the the stack using a non-canonical pointer will
suffer #SS, but ERETU doesn't modify the userspace stack AFAICT.  I
can't see anything in the ERETU pseudocode in the FRED spec that
mentions a canonical check or memory access using %rsp.

~Andrew

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 2, 2025 4:42:27 PM PDT, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space RSP
>
>How?  Or to phrase it differently, I hope not.
>
>%rsp is a 64bit value and does not have canonical restrictions elsewhere
>in the architecture, so far as I'm aware.  IRET really can restore a
>non-canonical %rsp, and userspace can run for an indeterminate period of
>time with a non-canonical %rsp as long as there are no stack accesses.
>
>Accesses relative to the the stack using a non-canonical pointer will
>suffer #SS, but ERETU doesn't modify the userspace stack AFAICT.  I
>can't see anything in the ERETU pseudocode in the FRED spec that
>mentions a canonical check or memory access using %rsp.
>
>~Andrew

You are right of course. Brainfart on my part.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[David Laight]

On Tue, 01 Jul 2025 19:06:10 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:
...
> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space
> RSP even in the absence of LASS, so if that is not currently handled that is an active bug.

Is that a fault in kernel space, or a fault in user space. 

Some of the traps for 'iret' happen after the transition to user space,
so the kernel doesn't have to handle them as special cases.
(I simplified (and fixed) one version of that code.)

	David

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[H. Peter Anvin]

On July 6, 2025 2:22:13 AM PDT, David Laight <david.laight.linux@gmail.com> wrote:
>On Tue, 01 Jul 2025 19:06:10 -0700
>"H. Peter Anvin" <hpa@zytor.com> wrote:
>...
>> Note: for a FRED system, ERETU can generate #SS for a non-canonical user space
>> RSP even in the absence of LASS, so if that is not currently handled that is an active bug.
>
>Is that a fault in kernel space, or a fault in user space. 
>
>Some of the traps for 'iret' happen after the transition to user space,
>so the kernel doesn't have to handle them as special cases.
>(I simplified (and fixed) one version of that code.)
>
>	David
>

It's a fault in user space. I had a brain fart and managed to forget that RSP is technically a GPR and as such is not limited to the VA width of the machine.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Tue, Jul 01, 2025 at 06:35:40PM -0700, Sohil Mehta wrote:
> On 7/1/2025 2:58 AM, Kirill A. Shutemov wrote:
> > LASS throws a #GP for any violations except for stack register accesses,
> > in which case it throws a #SS instead. Handle this similarly to how other
> > LASS violations are handled.
> > 
> 
> Maybe I've misunderstood something:
> 
> Is the underlying assumption here that #SS were previously only
> generated by userspace, but now they can also be generated by the
> kernel? And we want the kernel generated #SS to behave the same as the #GP?

It can be generated by both kernel and userspace if RSP gets corrupted.

So far, do_error_trap() did the trick, handling what has to be handled.
LASS requires a bit more, though.

> 
> > In case of FRED, before handling #SS as LASS violation, kernel has to
> > check if there's a fixup for the exception. It can address #SS due to
> > invalid user context on ERETU. See 5105e7687ad3 ("x86/fred: Fixup
> > fault on ERETU by jumping to fred_entrypoint_user") for more details.
> > 
> > Co-developed-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/kernel/traps.c | 39 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index ceb091f17a5b..f9ca5b911141 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -418,12 +418,6 @@ DEFINE_IDTENTRY_ERRORCODE(exc_segment_not_present)
> >  		      SIGBUS, 0, NULL);
> >  }
> >  
> > -DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> > -{
> > -	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> > -		      0, NULL);
> > -}
> > -
> >  DEFINE_IDTENTRY_ERRORCODE(exc_alignment_check)
> >  {
> >  	char *str = "alignment check";
> > @@ -866,6 +860,39 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> >  	cond_local_irq_disable(regs);
> >  }
> >  
> > +#define SSFSTR "stack segment fault"
> > +
> > +DEFINE_IDTENTRY_ERRORCODE(exc_stack_segment)
> > +{
> > +	if (user_mode(regs))
> > +		goto error_trap;
> > +
> > +	if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> > +	    fixup_exception(regs, X86_TRAP_SS, error_code, 0))
> > +		return;
> > +
> > +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> > +		enum kernel_exc_hint hint;
> > +		unsigned long exc_addr;
> > +
> > +		hint = get_kernel_exc_address(regs, &exc_addr);
> > +		if (hint != EXC_NO_HINT) {
> 
> The brackets are not needed for singular statements. Also the max line
> length is longer now. You can fit this all in a single line.

I think line split if justified. It is 120 characters long otherwise.
And with multi-line statement, brackets help readability.

I don't see a reason to change it.

> > +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
> > +			       exc_addr);
> > +		}
> > +
> 
> > +		if (hint != EXC_NON_CANONICAL)
> > +			exc_addr = 0;
> > +
> > +		die_addr(SSFSTR, regs, error_code, exc_addr);
> 
> The variable names in die_addr() should be generalized as well. They
> seem to assume the caller to be a #GP handler.

Okay, will fold into "x86/traps: Generalize #GP address decode and hint
code".

> > +		return;
> > +	}
> > +
> > +error_trap:
> > +	do_error_trap(regs, error_code, "stack segment", X86_TRAP_SS, SIGBUS,
> > +		      0, NULL);
> > +}
> > +
> >  static bool do_int3(struct pt_regs *regs)
> >  {
> >  	int res;
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Sohil Mehta]

On 7/2/2025 6:27 AM, Kirill A. Shutemov wrote:
>>> +
>>> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>>> +		enum kernel_exc_hint hint;
>>> +		unsigned long exc_addr;
>>> +
>>> +		hint = get_kernel_exc_address(regs, &exc_addr);
>>> +		if (hint != EXC_NO_HINT) {
>>
>> The brackets are not needed for singular statements. Also the max line
>> length is longer now. You can fit this all in a single line.
> 
> I think line split if justified. It is 120 characters long otherwise.
> And with multi-line statement, brackets help readability.
> 

Are you sure? Below ends at 90 characters for me, including the three
8-char tabs:

printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint], exc_addr);

> I don't see a reason to change it.

To reduce indentation, you could also do:

	if (!cpu_feature_enabled(X86_FEATURE_LASS))
		goto error_trap;

> 
>>> +			printk(SSFSTR ", %s 0x%lx", kernel_exc_hint_help[hint],
>>> +			       exc_addr);
>>> +		}
>>> +
>>

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Wed, Jul 02, 2025 at 10:56:25AM -0700, Sohil Mehta wrote:
> To reduce indentation, you could also do:
> 
> 	if (!cpu_feature_enabled(X86_FEATURE_LASS))
> 		goto error_trap;

Okay, will do this.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Sohil Mehta]

On 7/2/2025 6:27 AM, Kirill A. Shutemov wrote:

>>
>> Maybe I've misunderstood something:
>>
>> Is the underlying assumption here that #SS were previously only
>> generated by userspace, but now they can also be generated by the
>> kernel? And we want the kernel generated #SS to behave the same as the #GP?
> 
> It can be generated by both kernel and userspace if RSP gets corrupted.
> 
> So far, do_error_trap() did the trick, handling what has to be handled.
> LASS requires a bit more, though.
> 
Thank you for the information! The discussion in the other thread helped
clarify my confusion about the new FRED specific fixup outside the LASS
check.

IIUC, for kernel generated #SS, the prior code in do_error_trap()
would've done a few things such as notify_die() and
cond_local_irq_enable() before calling die().

The new code now directly calls die_addr(). Are we changing the behavior
for legacy kernel #SS? Also, why don't we need those calls for the new
LASS #SS?

I apologize if the questions seem too naive. I am finding the exception
handling code a bit convoluted to understand. In general, I would
suggest adding some code comments at least for the new code to help
ignorant folks like me looking at this in the future.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Wed, Jul 02, 2025 at 01:05:17PM -0700, Sohil Mehta wrote:
> On 7/2/2025 6:27 AM, Kirill A. Shutemov wrote:
> 
> >>
> >> Maybe I've misunderstood something:
> >>
> >> Is the underlying assumption here that #SS were previously only
> >> generated by userspace, but now they can also be generated by the
> >> kernel? And we want the kernel generated #SS to behave the same as the #GP?
> > 
> > It can be generated by both kernel and userspace if RSP gets corrupted.
> > 
> > So far, do_error_trap() did the trick, handling what has to be handled.
> > LASS requires a bit more, though.
> > 
> Thank you for the information! The discussion in the other thread helped
> clarify my confusion about the new FRED specific fixup outside the LASS
> check.
> 
> IIUC, for kernel generated #SS, the prior code in do_error_trap()
> would've done a few things such as notify_die() and
> cond_local_irq_enable() before calling die().

cond_local_irq_enable() need to happen if we want to do something
sleepable during exception handling. It is not the case here.

notify_die() will be called die_addr()->__die_body()->notify_die().

> The new code now directly calls die_addr(). Are we changing the behavior
> for legacy kernel #SS? Also, why don't we need those calls for the new
> LASS #SS?

do_error_trap() provides catch-all handling for unallowed-thing-happened
exception handling in either kernel or userspace.

We can take simpler path for fatal in-kernel exception. Following #GP
logic matches what we need.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Sohil Mehta]

On 7/3/2025 4:31 AM, Kirill A. Shutemov wrote:

> 
> cond_local_irq_enable() need to happen if we want to do something
> sleepable during exception handling. It is not the case here.
> 

Makes sense.

> notify_die() will be called die_addr()->__die_body()->notify_die().

The notify_die() within __die_body() is slightly different from the one
called from the exception handlers.

__die_body():
notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV)

exc_alignment_check():
notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_AC, SIGBUS)

I believe we should include a #SS specific notify before calling
die_addr(). Similar to exc_alignment_check() which dies on kernel
exceptions.


if (notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_SS, SIGBUS) ==
NOTIFY_STOP)
	return;


This way the behavior remains consistent with other exception handlers
as well as with/without LASS.

Re: [PATCHv8 14/17] x86/traps: Handle LASS thrown #SS

[Kirill A. Shutemov]

On Thu, Jul 03, 2025 at 01:12:11PM -0700, Sohil Mehta wrote:
> I believe we should include a #SS specific notify before calling
> die_addr(). Similar to exc_alignment_check() which dies on kernel
> exceptions.

You are right. Will update this.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Tue,  1 Jul 2025 12:58:31 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> Extract memcpy and memset functions from copy_user_generic() and
> __clear_user().
> 
> They can be used as inline memcpy and memset instead of the GCC builtins
> whenever necessary. LASS requires them to handle text_poke.

Except they contain the fault handlers so aren't generic calls.

> 
> Originally-by: Peter Zijlstra <peterz@infradead.org>
> Link: https://lore.kernel.org/all/20241029184840.GJ14555@noisy.programming.kicks-ass.net/
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/include/asm/string.h     | 46 +++++++++++++++++++++++++++++++
>  arch/x86/include/asm/uaccess_64.h | 38 +++++++------------------
>  arch/x86/lib/clear_page_64.S      | 13 +++++++--
>  3 files changed, 67 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/include/asm/string.h b/arch/x86/include/asm/string.h
> index c3c2c1914d65..17f6b5bfa8c1 100644
> --- a/arch/x86/include/asm/string.h
> +++ b/arch/x86/include/asm/string.h
> @@ -1,6 +1,52 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_STRING_H
> +#define _ASM_X86_STRING_H
> +
> +#include <asm/asm.h>
> +#include <asm/alternative.h>
> +#include <asm/cpufeatures.h>
> +
>  #ifdef CONFIG_X86_32
>  # include <asm/string_32.h>
>  #else
>  # include <asm/string_64.h>
>  #endif
> +
> +#ifdef CONFIG_X86_64
> +#define ALT_64(orig, alt, feat) ALTERNATIVE(orig, alt, feat)
> +#else
> +#define ALT_64(orig, alt, feat) orig "\n"
> +#endif
> +
> +static __always_inline void *__inline_memcpy(void *to, const void *from, size_t len)
> +{
> +	void *ret = to;
> +
> +	asm volatile("1:\n\t"
> +		     ALT_64("rep movsb",
> +			    "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
> +		     "2:\n\t"
> +		     _ASM_EXTABLE_UA(1b, 2b)
> +		     : "+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
> +		     : : "memory", _ASM_AX);
> +
> +	return ret + len;
> +}
> +
> +static __always_inline void *__inline_memset(void *addr, int v, size_t len)
> +{
> +	void *ret = addr;
> +
> +	asm volatile("1:\n\t"
> +		     ALT_64("rep stosb",
> +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> +		     "2:\n\t"
> +		     _ASM_EXTABLE_UA(1b, 2b)
> +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> +		     : "a" ((uint8_t)v)

You shouldn't need the (uint8_t) cast (should that be (u8) anyway).
At best it doesn't matter, at worst it will add code to mask with 0xff.

> +		     : "memory", _ASM_SI, _ASM_DX);
> +
> +	return ret + len;
> +}
> +
> +#endif /* _ASM_X86_STRING_H */
> diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> index c8a5ae35c871..eb531e13e659 100644
> --- a/arch/x86/include/asm/uaccess_64.h
> +++ b/arch/x86/include/asm/uaccess_64.h
> @@ -13,6 +13,7 @@
>  #include <asm/page.h>
>  #include <asm/percpu.h>
>  #include <asm/runtime-const.h>
> +#include <asm/string.h>
>  
>  /*
>   * Virtual variable: there's no actual backing store for this,
> @@ -118,21 +119,12 @@ rep_movs_alternative(void *to, const void *from, unsigned len);
>  static __always_inline __must_check unsigned long
>  copy_user_generic(void *to, const void *from, unsigned long len)
>  {
> +	void *ret;
> +
>  	stac();
> -	/*
> -	 * If CPU has FSRM feature, use 'rep movs'.
> -	 * Otherwise, use rep_movs_alternative.
> -	 */
> -	asm volatile(
> -		"1:\n\t"
> -		ALTERNATIVE("rep movsb",
> -			    "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
> -		"2:\n"
> -		_ASM_EXTABLE_UA(1b, 2b)
> -		:"+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
> -		: : "memory", "rax");
> +	ret = __inline_memcpy(to, from, len);
>  	clac();
> -	return len;
> +	return ret - to;
>  }
>  
>  static __always_inline __must_check unsigned long
> @@ -178,25 +170,15 @@ rep_stos_alternative(void __user *addr, unsigned long len);
>  
>  static __always_inline __must_check unsigned long __clear_user(void __user *addr, unsigned long size)
>  {
> +	void *ptr = (__force void *)addr;
> +	void *ret;
> +
>  	might_fault();
>  	stac();
> -
> -	/*
> -	 * No memory constraint because it doesn't change any memory gcc
> -	 * knows about.
> -	 */
> -	asm volatile(
> -		"1:\n\t"
> -		ALTERNATIVE("rep stosb",
> -			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRS))
> -		"2:\n"
> -	       _ASM_EXTABLE_UA(1b, 2b)
> -	       : "+c" (size), "+D" (addr), ASM_CALL_CONSTRAINT
> -	       : "a" (0));
> -
> +	ret = __inline_memset(ptr, 0, size);
>  	clac();
>  
> -	return size;
> +	return ret - ptr;
>  }
>  
>  static __always_inline unsigned long clear_user(void __user *to, unsigned long n)
> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> index a508e4a8c66a..47b613690f84 100644
> --- a/arch/x86/lib/clear_page_64.S
> +++ b/arch/x86/lib/clear_page_64.S
> @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
>  EXPORT_SYMBOL_GPL(clear_page_erms)
>  
>  /*
> - * Default clear user-space.
> + * Default memset.
>   * Input:
>   * rdi destination
> + * rsi scratch
>   * rcx count
> - * rax is zero
> + * al is value
>   *
>   * Output:
>   * rcx: uncleared bytes or 0 if successful.
> + * rdx: clobbered
>   */
>  SYM_FUNC_START(rep_stos_alternative)
>  	ANNOTATE_NOENDBR
> +
> +	movzbq %al, %rsi
> +	movabs $0x0101010101010101, %rax
> +
> +	/* RDX:RAX = RAX * RSI */
> +	mulq %rsi

NAK - you can't do that here.
Neither %rsi nor %rdx can be trashed.
The function has a very explicit calling convention.

It is also almost certainly a waste of time.
Pretty much all the calls will be for a constant 0x00.
Rename it all memzero() ...

	David

> +
>  	cmpq $64,%rcx
>  	jae .Lunrolled
>  

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Kirill A. Shutemov]

On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:
> On Tue,  1 Jul 2025 12:58:31 +0300
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > Extract memcpy and memset functions from copy_user_generic() and
> > __clear_user().
> > 
> > They can be used as inline memcpy and memset instead of the GCC builtins
> > whenever necessary. LASS requires them to handle text_poke.
> 
> Except they contain the fault handlers so aren't generic calls.

That's true. I will add a comment to clarify it.

> > Originally-by: Peter Zijlstra <peterz@infradead.org>
> > Link: https://lore.kernel.org/all/20241029184840.GJ14555@noisy.programming.kicks-ass.net/
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  arch/x86/include/asm/string.h     | 46 +++++++++++++++++++++++++++++++
> >  arch/x86/include/asm/uaccess_64.h | 38 +++++++------------------
> >  arch/x86/lib/clear_page_64.S      | 13 +++++++--
> >  3 files changed, 67 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/string.h b/arch/x86/include/asm/string.h
> > index c3c2c1914d65..17f6b5bfa8c1 100644
> > --- a/arch/x86/include/asm/string.h
> > +++ b/arch/x86/include/asm/string.h
> > @@ -1,6 +1,52 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_X86_STRING_H
> > +#define _ASM_X86_STRING_H
> > +
> > +#include <asm/asm.h>
> > +#include <asm/alternative.h>
> > +#include <asm/cpufeatures.h>
> > +
> >  #ifdef CONFIG_X86_32
> >  # include <asm/string_32.h>
> >  #else
> >  # include <asm/string_64.h>
> >  #endif
> > +
> > +#ifdef CONFIG_X86_64
> > +#define ALT_64(orig, alt, feat) ALTERNATIVE(orig, alt, feat)
> > +#else
> > +#define ALT_64(orig, alt, feat) orig "\n"
> > +#endif
> > +
> > +static __always_inline void *__inline_memcpy(void *to, const void *from, size_t len)
> > +{
> > +	void *ret = to;
> > +
> > +	asm volatile("1:\n\t"
> > +		     ALT_64("rep movsb",
> > +			    "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > +		     "2:\n\t"
> > +		     _ASM_EXTABLE_UA(1b, 2b)
> > +		     : "+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
> > +		     : : "memory", _ASM_AX);
> > +
> > +	return ret + len;
> > +}
> > +
> > +static __always_inline void *__inline_memset(void *addr, int v, size_t len)
> > +{
> > +	void *ret = addr;
> > +
> > +	asm volatile("1:\n\t"
> > +		     ALT_64("rep stosb",
> > +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > +		     "2:\n\t"
> > +		     _ASM_EXTABLE_UA(1b, 2b)
> > +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> > +		     : "a" ((uint8_t)v)
> 
> You shouldn't need the (uint8_t) cast (should that be (u8) anyway).
> At best it doesn't matter, at worst it will add code to mask with 0xff.

Right, will drop.

> > +		     : "memory", _ASM_SI, _ASM_DX);
> > +
> > +	return ret + len;
> > +}
> > +
> > +#endif /* _ASM_X86_STRING_H */
> > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> > index c8a5ae35c871..eb531e13e659 100644
> > --- a/arch/x86/include/asm/uaccess_64.h
> > +++ b/arch/x86/include/asm/uaccess_64.h
> > @@ -13,6 +13,7 @@
> >  #include <asm/page.h>
> >  #include <asm/percpu.h>
> >  #include <asm/runtime-const.h>
> > +#include <asm/string.h>
> >  
> >  /*
> >   * Virtual variable: there's no actual backing store for this,
> > @@ -118,21 +119,12 @@ rep_movs_alternative(void *to, const void *from, unsigned len);
> >  static __always_inline __must_check unsigned long
> >  copy_user_generic(void *to, const void *from, unsigned long len)
> >  {
> > +	void *ret;
> > +
> >  	stac();
> > -	/*
> > -	 * If CPU has FSRM feature, use 'rep movs'.
> > -	 * Otherwise, use rep_movs_alternative.
> > -	 */
> > -	asm volatile(
> > -		"1:\n\t"
> > -		ALTERNATIVE("rep movsb",
> > -			    "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > -		"2:\n"
> > -		_ASM_EXTABLE_UA(1b, 2b)
> > -		:"+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
> > -		: : "memory", "rax");
> > +	ret = __inline_memcpy(to, from, len);
> >  	clac();
> > -	return len;
> > +	return ret - to;
> >  }
> >  
> >  static __always_inline __must_check unsigned long
> > @@ -178,25 +170,15 @@ rep_stos_alternative(void __user *addr, unsigned long len);
> >  
> >  static __always_inline __must_check unsigned long __clear_user(void __user *addr, unsigned long size)
> >  {
> > +	void *ptr = (__force void *)addr;
> > +	void *ret;
> > +
> >  	might_fault();
> >  	stac();
> > -
> > -	/*
> > -	 * No memory constraint because it doesn't change any memory gcc
> > -	 * knows about.
> > -	 */
> > -	asm volatile(
> > -		"1:\n\t"
> > -		ALTERNATIVE("rep stosb",
> > -			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRS))
> > -		"2:\n"
> > -	       _ASM_EXTABLE_UA(1b, 2b)
> > -	       : "+c" (size), "+D" (addr), ASM_CALL_CONSTRAINT
> > -	       : "a" (0));
> > -
> > +	ret = __inline_memset(ptr, 0, size);
> >  	clac();
> >  
> > -	return size;
> > +	return ret - ptr;
> >  }
> >  
> >  static __always_inline unsigned long clear_user(void __user *to, unsigned long n)
> > diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> > index a508e4a8c66a..47b613690f84 100644
> > --- a/arch/x86/lib/clear_page_64.S
> > +++ b/arch/x86/lib/clear_page_64.S
> > @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
> >  EXPORT_SYMBOL_GPL(clear_page_erms)
> >  
> >  /*
> > - * Default clear user-space.
> > + * Default memset.
> >   * Input:
> >   * rdi destination
> > + * rsi scratch
> >   * rcx count
> > - * rax is zero
> > + * al is value
> >   *
> >   * Output:
> >   * rcx: uncleared bytes or 0 if successful.
> > + * rdx: clobbered
> >   */
> >  SYM_FUNC_START(rep_stos_alternative)
> >  	ANNOTATE_NOENDBR
> > +
> > +	movzbq %al, %rsi
> > +	movabs $0x0101010101010101, %rax
> > +
> > +	/* RDX:RAX = RAX * RSI */
> > +	mulq %rsi
> 
> NAK - you can't do that here.
> Neither %rsi nor %rdx can be trashed.
> The function has a very explicit calling convention.

What calling convention? We change the only caller to confirm to this.

> It is also almost certainly a waste of time.
> Pretty much all the calls will be for a constant 0x00.
> Rename it all memzero() ...

text_poke_memset() is not limited to zeroing.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Thu, 3 Jul 2025 13:39:57 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:
> > On Tue,  1 Jul 2025 12:58:31 +0300
> > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> >   
> > > Extract memcpy and memset functions from copy_user_generic() and
> > > __clear_user().
> > > 
> > > They can be used as inline memcpy and memset instead of the GCC builtins
> > > whenever necessary. LASS requires them to handle text_poke.  
> > 
> > Except they contain the fault handlers so aren't generic calls.  
> 
> That's true. I will add a comment to clarify it.

They need renaming.

...
> > > diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> > > index a508e4a8c66a..47b613690f84 100644
> > > --- a/arch/x86/lib/clear_page_64.S
> > > +++ b/arch/x86/lib/clear_page_64.S
> > > @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
> > >  EXPORT_SYMBOL_GPL(clear_page_erms)
> > >  
> > >  /*
> > > - * Default clear user-space.
> > > + * Default memset.
> > >   * Input:
> > >   * rdi destination
> > > + * rsi scratch
> > >   * rcx count
> > > - * rax is zero
> > > + * al is value
> > >   *
> > >   * Output:
> > >   * rcx: uncleared bytes or 0 if successful.
> > > + * rdx: clobbered
> > >   */
> > >  SYM_FUNC_START(rep_stos_alternative)
> > >  	ANNOTATE_NOENDBR
> > > +
> > > +	movzbq %al, %rsi
> > > +	movabs $0x0101010101010101, %rax
> > > +
> > > +	/* RDX:RAX = RAX * RSI */
> > > +	mulq %rsi  
> > 
> > NAK - you can't do that here.
> > Neither %rsi nor %rdx can be trashed.
> > The function has a very explicit calling convention.  
> 
> What calling convention? We change the only caller to confirm to this.

The one that is implicit in:

> > > +	asm volatile("1:\n\t"
> > > +		     ALT_64("rep stosb",
> > > +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > > +		     "2:\n\t"
> > > +		     _ASM_EXTABLE_UA(1b, 2b)
> > > +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> > > +		     : "a" ((uint8_t)v)

The called function is only allowed to change the registers that
'rep stosb' uses - except it can access (but not change)
all of %rax - not just %al.

See: https://godbolt.org/z/3fnrT3x9r
In particular note that 'do_mset' must not change %rax.

This is very specific and is done so that the compiler can use
all the registers.

> > It is also almost certainly a waste of time.
> > Pretty much all the calls will be for a constant 0x00.
> > Rename it all memzero() ...  
> 
> text_poke_memset() is not limited to zeroing.

But you don't want the overhead of extending the constant
on all the calls - never mind reserving %rdx to do it.
Maybe define a function that requires the caller to have
done the 'dirty work' - so any code that wants memzero()
just passes zero.
Or do the multiply in the C code where it will get optimised
away for constant zero.
You do get the multiply for the 'rep stosb' case - but that
is always going to be true unless you complicate things further.  

	David

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Vegard Nossum]

On 03/07/2025 14:15, David Laight wrote:
> On Thu, 3 Jul 2025 13:39:57 +0300
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
>> On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:
>>> On Tue,  1 Jul 2025 12:58:31 +0300
>>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
>>>> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
>>>> index a508e4a8c66a..47b613690f84 100644
>>>> --- a/arch/x86/lib/clear_page_64.S
>>>> +++ b/arch/x86/lib/clear_page_64.S
>>>> @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
>>>>   EXPORT_SYMBOL_GPL(clear_page_erms)
>>>>   
>>>>   /*
>>>> - * Default clear user-space.
>>>> + * Default memset.
>>>>    * Input:
>>>>    * rdi destination
>>>> + * rsi scratch
>>>>    * rcx count
>>>> - * rax is zero
>>>> + * al is value
>>>>    *
>>>>    * Output:
>>>>    * rcx: uncleared bytes or 0 if successful.
>>>> + * rdx: clobbered
>>>>    */
>>>>   SYM_FUNC_START(rep_stos_alternative)
>>>>   	ANNOTATE_NOENDBR
>>>> +
>>>> +	movzbq %al, %rsi
>>>> +	movabs $0x0101010101010101, %rax
>>>> +
>>>> +	/* RDX:RAX = RAX * RSI */
>>>> +	mulq %rsi
>>>
>>> NAK - you can't do that here.
>>> Neither %rsi nor %rdx can be trashed.
>>> The function has a very explicit calling convention.

That's why we have the clobbers... see below

>> What calling convention? We change the only caller to confirm to this.
> 
> The one that is implicit in:
> 
>>>> +	asm volatile("1:\n\t"
>>>> +		     ALT_64("rep stosb",
>>>> +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
>>>> +		     "2:\n\t"
>>>> +		     _ASM_EXTABLE_UA(1b, 2b)
>>>> +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
>>>> +		     : "a" ((uint8_t)v)
> 
> The called function is only allowed to change the registers that
> 'rep stosb' uses - except it can access (but not change)
> all of %rax - not just %al.
> 
> See: https://godbolt.org/z/3fnrT3x9r
> In particular note that 'do_mset' must not change %rax.
> 
> This is very specific and is done so that the compiler can use
> all the registers.

I feel like you trimmed off the clobbers from the asm() in the context
above. For reference, it is:

+		     : "memory", _ASM_SI, _ASM_DX);

I'm not saying this can't be optimized, but that doesn't seem to be your
complaint -- you say "the called function is only allowed to change
...", but this is not true when we have the clobbers, right?

This is exactly what I fixed with my v7 fixlet to this patch:

https://lore.kernel.org/all/1b96b0ca-5c14-4271-86c1-c305bf052b16@oracle.com/


Vegard

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Thu, 3 Jul 2025 15:33:16 +0200
Vegard Nossum <vegard.nossum@oracle.com> wrote:

> On 03/07/2025 14:15, David Laight wrote:
> > On Thu, 3 Jul 2025 13:39:57 +0300
> > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:  
> >> On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:  
> >>> On Tue,  1 Jul 2025 12:58:31 +0300
> >>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:  
> >>>> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> >>>> index a508e4a8c66a..47b613690f84 100644
> >>>> --- a/arch/x86/lib/clear_page_64.S
> >>>> +++ b/arch/x86/lib/clear_page_64.S
> >>>> @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
> >>>>   EXPORT_SYMBOL_GPL(clear_page_erms)
> >>>>   
> >>>>   /*
> >>>> - * Default clear user-space.
> >>>> + * Default memset.
> >>>>    * Input:
> >>>>    * rdi destination
> >>>> + * rsi scratch
> >>>>    * rcx count
> >>>> - * rax is zero
> >>>> + * al is value
> >>>>    *
> >>>>    * Output:
> >>>>    * rcx: uncleared bytes or 0 if successful.
> >>>> + * rdx: clobbered
> >>>>    */
> >>>>   SYM_FUNC_START(rep_stos_alternative)
> >>>>   	ANNOTATE_NOENDBR
> >>>> +
> >>>> +	movzbq %al, %rsi
> >>>> +	movabs $0x0101010101010101, %rax
> >>>> +
> >>>> +	/* RDX:RAX = RAX * RSI */
> >>>> +	mulq %rsi  
> >>>
> >>> NAK - you can't do that here.
> >>> Neither %rsi nor %rdx can be trashed.
> >>> The function has a very explicit calling convention.  
> 
> That's why we have the clobbers... see below
> 
> >> What calling convention? We change the only caller to confirm to this.  
> > 
> > The one that is implicit in:
> >   
> >>>> +	asm volatile("1:\n\t"
> >>>> +		     ALT_64("rep stosb",
> >>>> +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> >>>> +		     "2:\n\t"
> >>>> +		     _ASM_EXTABLE_UA(1b, 2b)
> >>>> +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> >>>> +		     : "a" ((uint8_t)v)  
> > 
> > The called function is only allowed to change the registers that
> > 'rep stosb' uses - except it can access (but not change)
> > all of %rax - not just %al.
> > 
> > See: https://godbolt.org/z/3fnrT3x9r
> > In particular note that 'do_mset' must not change %rax.
> > 
> > This is very specific and is done so that the compiler can use
> > all the registers.  
> 
> I feel like you trimmed off the clobbers from the asm() in the context
> above. For reference, it is:
> 
> +		     : "memory", _ASM_SI, _ASM_DX);

I'm sure they weren't there...

Enough clobbers will 'un-break' it - but that isn't the point.
Linux will reject the patch if he reads it.
The whole point about the function is that it is as direct a replacement
for 'rep stos/movsb' as possible.  

> 
> I'm not saying this can't be optimized, but that doesn't seem to be your
> complaint -- you say "the called function is only allowed to change
> ...", but this is not true when we have the clobbers, right?

You can't change %rax either - not without a clobber.

Oh, and even with your version you only clobbers for %rax and %rdx.
There is no need to use both %rsi and %rdx.

The performance is a different problem.
And the extra clobbers are likely to matter.
x86 really doesn't have many registers.

	David

> 
> This is exactly what I fixed with my v7 fixlet to this patch:
> 
> https://lore.kernel.org/all/1b96b0ca-5c14-4271-86c1-c305bf052b16@oracle.com/
> 
> 
> Vegard

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Kirill A. Shutemov]

On Thu, Jul 03, 2025 at 01:15:52PM +0100, David Laight wrote:
> On Thu, 3 Jul 2025 13:39:57 +0300
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:
> > > On Tue,  1 Jul 2025 12:58:31 +0300
> > > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > >   
> > > > Extract memcpy and memset functions from copy_user_generic() and
> > > > __clear_user().
> > > > 
> > > > They can be used as inline memcpy and memset instead of the GCC builtins
> > > > whenever necessary. LASS requires them to handle text_poke.  
> > > 
> > > Except they contain the fault handlers so aren't generic calls.  
> > 
> > That's true. I will add a comment to clarify it.
> 
> They need renaming.

__inline_memcpy/memset_safe()?

> ...
> > > > diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> > > > index a508e4a8c66a..47b613690f84 100644
> > > > --- a/arch/x86/lib/clear_page_64.S
> > > > +++ b/arch/x86/lib/clear_page_64.S
> > > > @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
> > > >  EXPORT_SYMBOL_GPL(clear_page_erms)
> > > >  
> > > >  /*
> > > > - * Default clear user-space.
> > > > + * Default memset.
> > > >   * Input:
> > > >   * rdi destination
> > > > + * rsi scratch
> > > >   * rcx count
> > > > - * rax is zero
> > > > + * al is value
> > > >   *
> > > >   * Output:
> > > >   * rcx: uncleared bytes or 0 if successful.
> > > > + * rdx: clobbered
> > > >   */
> > > >  SYM_FUNC_START(rep_stos_alternative)
> > > >  	ANNOTATE_NOENDBR
> > > > +
> > > > +	movzbq %al, %rsi
> > > > +	movabs $0x0101010101010101, %rax
> > > > +
> > > > +	/* RDX:RAX = RAX * RSI */
> > > > +	mulq %rsi  
> > > 
> > > NAK - you can't do that here.
> > > Neither %rsi nor %rdx can be trashed.
> > > The function has a very explicit calling convention.  
> > 
> > What calling convention? We change the only caller to confirm to this.
> 
> The one that is implicit in:
> 
> > > > +	asm volatile("1:\n\t"
> > > > +		     ALT_64("rep stosb",
> > > > +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > > > +		     "2:\n\t"
> > > > +		     _ASM_EXTABLE_UA(1b, 2b)
> > > > +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> > > > +		     : "a" ((uint8_t)v)
> 
> The called function is only allowed to change the registers that
> 'rep stosb' uses - except it can access (but not change)
> all of %rax - not just %al.
> 
> See: https://godbolt.org/z/3fnrT3x9r
> In particular note that 'do_mset' must not change %rax.
> 
> This is very specific and is done so that the compiler can use
> all the registers.

Okay, I see what you are saying.

> > > It is also almost certainly a waste of time.
> > > Pretty much all the calls will be for a constant 0x00.
> > > Rename it all memzero() ...  
> > 
> > text_poke_memset() is not limited to zeroing.
> 
> But you don't want the overhead of extending the constant
> on all the calls - never mind reserving %rdx to do it.
> Maybe define a function that requires the caller to have
> done the 'dirty work' - so any code that wants memzero()
> just passes zero.
> Or do the multiply in the C code where it will get optimised
> away for constant zero.
> You do get the multiply for the 'rep stosb' case - but that
> is always going to be true unless you complicate things further.  

The patch below seems to do the trick: compiler optimizes out the
multiplication for v == 0.

It would be nice to avoid it for X86_FEATURE_FSRM, but we cannot use
cpu_feature_enabled() here as <asm/cpufeature.h> depends on
<asm/string.h>.

I cannot say I like the result.

Any suggestions?

diff --git a/arch/x86/include/asm/string.h b/arch/x86/include/asm/string.h
index becb9ee3bc8a..c7644a6f426b 100644
--- a/arch/x86/include/asm/string.h
+++ b/arch/x86/include/asm/string.h
@@ -35,16 +35,27 @@ static __always_inline void *__inline_memcpy(void *to, const void *from, size_t
 
 static __always_inline void *__inline_memset(void *addr, int v, size_t len)
 {
+	unsigned long val = v;
 	void *ret = addr;
 
+	if (IS_ENABLED(CONFIG_X86_64)) {
+		/*
+		 * Fill all bytes by value in byte 0.
+		 *
+		 * To be used in rep_stos_alternative()i
+		 */
+		val &= 0xff;
+		val *= 0x0101010101010101;
+	}
+
 	asm volatile("1:\n\t"
 		     ALT_64("rep stosb",
 			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
 		     "2:\n\t"
 		     _ASM_EXTABLE_UA(1b, 2b)
 		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
-		     : "a" (v)
-		     : "memory", _ASM_SI, _ASM_DX);
+		     : "a" (val)
+		     : "memory");
 
 	return ret + len;
 }
diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
index 47b613690f84..3ef7d796deb3 100644
--- a/arch/x86/lib/clear_page_64.S
+++ b/arch/x86/lib/clear_page_64.S
@@ -58,23 +58,15 @@ EXPORT_SYMBOL_GPL(clear_page_erms)
  * Default memset.
  * Input:
  * rdi destination
- * rsi scratch
  * rcx count
  * al is value
  *
  * Output:
  * rcx: uncleared bytes or 0 if successful.
- * rdx: clobbered
  */
 SYM_FUNC_START(rep_stos_alternative)
 	ANNOTATE_NOENDBR
 
-	movzbq %al, %rsi
-	movabs $0x0101010101010101, %rax
-
-	/* RDX:RAX = RAX * RSI */
-	mulq %rsi
-
 	cmpq $64,%rcx
 	jae .Lunrolled
 
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Thu, 3 Jul 2025 17:10:34 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> On Thu, Jul 03, 2025 at 01:15:52PM +0100, David Laight wrote:
> > On Thu, 3 Jul 2025 13:39:57 +0300
> > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> >   
> > > On Thu, Jul 03, 2025 at 09:44:17AM +0100, David Laight wrote:  
> > > > On Tue,  1 Jul 2025 12:58:31 +0300
> > > > "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > > >     
> > > > > Extract memcpy and memset functions from copy_user_generic() and
> > > > > __clear_user().
> > > > > 
> > > > > They can be used as inline memcpy and memset instead of the GCC builtins
> > > > > whenever necessary. LASS requires them to handle text_poke.    
> > > > 
> > > > Except they contain the fault handlers so aren't generic calls.    
> > > 
> > > That's true. I will add a comment to clarify it.  
> > 
> > They need renaming.  
> 
> __inline_memcpy/memset_safe()?

'safe' against what :-)
They can't be used for user accesses without access_ok() and clac.
The get/put_user variants without access_ok() have _unsafe() suffix.

> 
> > ...  
> > > > > diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> > > > > index a508e4a8c66a..47b613690f84 100644
> > > > > --- a/arch/x86/lib/clear_page_64.S
> > > > > +++ b/arch/x86/lib/clear_page_64.S
> > > > > @@ -55,17 +55,26 @@ SYM_FUNC_END(clear_page_erms)
> > > > >  EXPORT_SYMBOL_GPL(clear_page_erms)
> > > > >  
> > > > >  /*
> > > > > - * Default clear user-space.
> > > > > + * Default memset.
> > > > >   * Input:
> > > > >   * rdi destination
> > > > > + * rsi scratch
> > > > >   * rcx count
> > > > > - * rax is zero
> > > > > + * al is value
> > > > >   *
> > > > >   * Output:
> > > > >   * rcx: uncleared bytes or 0 if successful.
> > > > > + * rdx: clobbered
> > > > >   */
> > > > >  SYM_FUNC_START(rep_stos_alternative)
> > > > >  	ANNOTATE_NOENDBR
> > > > > +
> > > > > +	movzbq %al, %rsi
> > > > > +	movabs $0x0101010101010101, %rax
> > > > > +
> > > > > +	/* RDX:RAX = RAX * RSI */
> > > > > +	mulq %rsi    
> > > > 
> > > > NAK - you can't do that here.
> > > > Neither %rsi nor %rdx can be trashed.
> > > > The function has a very explicit calling convention.    
> > > 
> > > What calling convention? We change the only caller to confirm to this.  
> > 
> > The one that is implicit in:
> >   
> > > > > +	asm volatile("1:\n\t"
> > > > > +		     ALT_64("rep stosb",
> > > > > +			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
> > > > > +		     "2:\n\t"
> > > > > +		     _ASM_EXTABLE_UA(1b, 2b)
> > > > > +		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> > > > > +		     : "a" ((uint8_t)v)  
> > 
> > The called function is only allowed to change the registers that
> > 'rep stosb' uses - except it can access (but not change)
> > all of %rax - not just %al.
> > 
> > See: https://godbolt.org/z/3fnrT3x9r
> > In particular note that 'do_mset' must not change %rax.
> > 
> > This is very specific and is done so that the compiler can use
> > all the registers.  
> 
> Okay, I see what you are saying.
> 
> > > > It is also almost certainly a waste of time.
> > > > Pretty much all the calls will be for a constant 0x00.
> > > > Rename it all memzero() ...    
> > > 
> > > text_poke_memset() is not limited to zeroing.  
> > 
> > But you don't want the overhead of extending the constant
> > on all the calls - never mind reserving %rdx to do it.
> > Maybe define a function that requires the caller to have
> > done the 'dirty work' - so any code that wants memzero()
> > just passes zero.
> > Or do the multiply in the C code where it will get optimised
> > away for constant zero.
> > You do get the multiply for the 'rep stosb' case - but that
> > is always going to be true unless you complicate things further.    
> 
> The patch below seems to do the trick: compiler optimizes out the
> multiplication for v == 0.
> 
> It would be nice to avoid it for X86_FEATURE_FSRM, but we cannot use
> cpu_feature_enabled() here as <asm/cpufeature.h> depends on
> <asm/string.h>.
> 
> I cannot say I like the result.
> 
> Any suggestions?
> 
> diff --git a/arch/x86/include/asm/string.h b/arch/x86/include/asm/string.h
> index becb9ee3bc8a..c7644a6f426b 100644
> --- a/arch/x86/include/asm/string.h
> +++ b/arch/x86/include/asm/string.h
> @@ -35,16 +35,27 @@ static __always_inline void *__inline_memcpy(void *to, const void *from, size_t
>  
>  static __always_inline void *__inline_memset(void *addr, int v, size_t len)
>  {
> +	unsigned long val = v;
>  	void *ret = addr;
>  
> +	if (IS_ENABLED(CONFIG_X86_64)) {
> +		/*
> +		 * Fill all bytes by value in byte 0.
> +		 *
> +		 * To be used in rep_stos_alternative()i
> +		 */
> +		val &= 0xff;
> +		val *= 0x0101010101010101;
> +	}

That won't compile for 32bit, and it needs the same thing done.
	val *= (unsigned long)0x0101010101010101ull;
should work.
I don't think you need the 'val &= 0xff', just rely on the caller
passing a valid value - nothing will break badly if it doesn't.

	David

> +
>  	asm volatile("1:\n\t"
>  		     ALT_64("rep stosb",
>  			    "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
>  		     "2:\n\t"
>  		     _ASM_EXTABLE_UA(1b, 2b)
>  		     : "+c" (len), "+D" (addr), ASM_CALL_CONSTRAINT
> -		     : "a" (v)
> -		     : "memory", _ASM_SI, _ASM_DX);
> +		     : "a" (val)
> +		     : "memory");
>  
>  	return ret + len;
>  }
> diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
> index 47b613690f84..3ef7d796deb3 100644
> --- a/arch/x86/lib/clear_page_64.S
> +++ b/arch/x86/lib/clear_page_64.S
> @@ -58,23 +58,15 @@ EXPORT_SYMBOL_GPL(clear_page_erms)
>   * Default memset.
>   * Input:
>   * rdi destination
> - * rsi scratch
>   * rcx count
>   * al is value
>   *
>   * Output:
>   * rcx: uncleared bytes or 0 if successful.
> - * rdx: clobbered
>   */
>  SYM_FUNC_START(rep_stos_alternative)
>  	ANNOTATE_NOENDBR
>  
> -	movzbq %al, %rsi
> -	movabs $0x0101010101010101, %rax
> -
> -	/* RDX:RAX = RAX * RSI */
> -	mulq %rsi
> -
>  	cmpq $64,%rcx
>  	jae .Lunrolled
>  

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Dave Hansen]

On 7/1/25 02:58, Kirill A. Shutemov wrote:
> Extract memcpy and memset functions from copy_user_generic() and
> __clear_user().
> 
> They can be used as inline memcpy and memset instead of the GCC builtins
> whenever necessary. LASS requires them to handle text_poke.

Why are we messing with the normal user copy functions? Code reuse is
great, but as you're discovering, the user copy code is highly
specialized and not that easy to reuse for other things.

Don't we just need a dirt simple chunk of code that does (logically):

	stac();
	asm("rep stosq...");
	clac();

Performance doesn't matter for text poking, right? It could be stosq or
anything else that you can inline. It could be a for() loop for all I
care as long as the compiler doesn't transform it into some out-of-line
memset. Right?

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Kirill A. Shutemov]

On Thu, Jul 03, 2025 at 10:13:44AM -0700, Dave Hansen wrote:
> On 7/1/25 02:58, Kirill A. Shutemov wrote:
> > Extract memcpy and memset functions from copy_user_generic() and
> > __clear_user().
> > 
> > They can be used as inline memcpy and memset instead of the GCC builtins
> > whenever necessary. LASS requires them to handle text_poke.
> 
> Why are we messing with the normal user copy functions? Code reuse is
> great, but as you're discovering, the user copy code is highly
> specialized and not that easy to reuse for other things.
> 
> Don't we just need a dirt simple chunk of code that does (logically):
> 
> 	stac();
> 	asm("rep stosq...");
> 	clac();
> 
> Performance doesn't matter for text poking, right? It could be stosq or
> anything else that you can inline. It could be a for() loop for all I
> care as long as the compiler doesn't transform it into some out-of-line
> memset. Right?

Yeah, performance doesn't matter for text poking. And this approach
simplifies the code quite a bit. I will use direct asm() for text poke.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Thu, 3 Jul 2025 10:13:44 -0700
Dave Hansen <dave.hansen@intel.com> wrote:

> On 7/1/25 02:58, Kirill A. Shutemov wrote:
> > Extract memcpy and memset functions from copy_user_generic() and
> > __clear_user().
> > 
> > They can be used as inline memcpy and memset instead of the GCC builtins
> > whenever necessary. LASS requires them to handle text_poke.  
> 
> Why are we messing with the normal user copy functions? Code reuse is
> great, but as you're discovering, the user copy code is highly
> specialized and not that easy to reuse for other things.
> 
> Don't we just need a dirt simple chunk of code that does (logically):
> 
> 	stac();
> 	asm("rep stosq...");
> 	clac();
> 
> Performance doesn't matter for text poking, right? It could be stosq or
> anything else that you can inline. It could be a for() loop for all I
> care as long as the compiler doesn't transform it into some out-of-line
> memset. Right?
> 

It doesn't even really matter if there is an out-of-line memset.
All you need to do is 'teach' objtool it isn't a problem.

Is this for the boot-time asm-alternatives?
In that case I wonder why a 'low' address is being used?
With LASS enabled using a low address on a life kernel would make it
harder for another cpu to leverage the writable code page, but
that isn't a requirement of LASS.

If it is being used for later instruction patching you need the
very careful instruction sequences and cpu synchronisation.
In that case I suspect you need to add conditional stac/clac
to the existing patching code (and teach objtool it is all ok).

	David

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[Kirill A. Shutemov]

On Sun, Jul 06, 2025 at 10:13:42AM +0100, David Laight wrote:
> On Thu, 3 Jul 2025 10:13:44 -0700
> Dave Hansen <dave.hansen@intel.com> wrote:
> 
> > On 7/1/25 02:58, Kirill A. Shutemov wrote:
> > > Extract memcpy and memset functions from copy_user_generic() and
> > > __clear_user().
> > > 
> > > They can be used as inline memcpy and memset instead of the GCC builtins
> > > whenever necessary. LASS requires them to handle text_poke.  
> > 
> > Why are we messing with the normal user copy functions? Code reuse is
> > great, but as you're discovering, the user copy code is highly
> > specialized and not that easy to reuse for other things.
> > 
> > Don't we just need a dirt simple chunk of code that does (logically):
> > 
> > 	stac();
> > 	asm("rep stosq...");
> > 	clac();
> > 
> > Performance doesn't matter for text poking, right? It could be stosq or
> > anything else that you can inline. It could be a for() loop for all I
> > care as long as the compiler doesn't transform it into some out-of-line
> > memset. Right?
> > 
> 
> It doesn't even really matter if there is an out-of-line memset.
> All you need to do is 'teach' objtool it isn't a problem.

PeterZ was not fan of the idead;

https://lore.kernel.org/all/20241029113611.GS14555@noisy.programming.kicks-ass.net/

> Is this for the boot-time asm-alternatives?

Not only boot-time. static_branches are switchable at runtime.

> In that case I wonder why a 'low' address is being used?
> With LASS enabled using a low address on a life kernel would make it
> harder for another cpu to leverage the writable code page, but
> that isn't a requirement of LASS.

Because kernel side of address space is shared across all CPU and we don't
want kernel code to be writable to all CPUs

> If it is being used for later instruction patching you need the
> very careful instruction sequences and cpu synchronisation.
> In that case I suspect you need to add conditional stac/clac
> to the existing patching code (and teach objtool it is all ok).

STAC/CLAC is conditional in text poke on LASS presence on the machine.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCHv8 02/17] x86/asm: Introduce inline memcpy and memset

[David Laight]

On Mon, 7 Jul 2025 11:02:06 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> On Sun, Jul 06, 2025 at 10:13:42AM +0100, David Laight wrote:
> > On Thu, 3 Jul 2025 10:13:44 -0700
> > Dave Hansen <dave.hansen@intel.com> wrote:
> >   
> > > On 7/1/25 02:58, Kirill A. Shutemov wrote:  
> > > > Extract memcpy and memset functions from copy_user_generic() and
> > > > __clear_user().
> > > > 
> > > > They can be used as inline memcpy and memset instead of the GCC builtins
> > > > whenever necessary. LASS requires them to handle text_poke.    
> > > 
> > > Why are we messing with the normal user copy functions? Code reuse is
> > > great, but as you're discovering, the user copy code is highly
> > > specialized and not that easy to reuse for other things.
> > > 
> > > Don't we just need a dirt simple chunk of code that does (logically):
> > > 
> > > 	stac();
> > > 	asm("rep stosq...");
> > > 	clac();
> > > 
> > > Performance doesn't matter for text poking, right? It could be stosq or
> > > anything else that you can inline. It could be a for() loop for all I
> > > care as long as the compiler doesn't transform it into some out-of-line
> > > memset. Right?
> > >   
> > 
> > It doesn't even really matter if there is an out-of-line memset.
> > All you need to do is 'teach' objtool it isn't a problem.  
> 
> PeterZ was not fan of the idead;
> 
> https://lore.kernel.org/all/20241029113611.GS14555@noisy.programming.kicks-ass.net/
> 
> > Is this for the boot-time asm-alternatives?  
> 
> Not only boot-time. static_branches are switchable at runtime.
> 
> > In that case I wonder why a 'low' address is being used?
> > With LASS enabled using a low address on a life kernel would make it
> > harder for another cpu to leverage the writable code page, but
> > that isn't a requirement of LASS.  
> 
> Because kernel side of address space is shared across all CPU and we don't
> want kernel code to be writable to all CPUs

So, as I said, it isn't a requirement for LASS.
Just something that LASS lets you do.
Although I'm sure there will be some odd effect of putting a 'supervisor'
page in the middle of 'user' pages.

Isn't there also (something like) kmap_local_page() that updates the local
page tables but doesn't broadcast the change?

> 
> > If it is being used for later instruction patching you need the
> > very careful instruction sequences and cpu synchronisation.
> > In that case I suspect you need to add conditional stac/clac
> > to the existing patching code (and teach objtool it is all ok).  
> 
> STAC/CLAC is conditional in text poke on LASS presence on the machine.

So just change the code to use byte copy loops with a volatile
destination pointer and all will be fine.

	David