[PATCH 0/2] stackleak: allow to specify arch specific stackleak poison function

[PATCH 0/2] stackleak: allow to specify arch specific stackleak poison function

[Heiko Carstens]

Factor out the code that fills the stack with the stackleak poison value in
order to allow architectures to provide a faster implementation.

Use this to provide an s390 specific implementation which can fill the
stack with the poison value much faster (factor of ~10 compared to the
current version).

Note that the s390 stackleak support is currently only available via
linux-next (as of today), and the s390 kernel tree at kernel.org[1].
Therefore, if there are no objections, I'd like to add these two patches to
the s390 tree, so they can go upstream via the next merge window together
with the s390 support.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features&id=b94c0ebb1ec752016a3e41bfb66bb51ea905e533

Thanks,
Heiko

Heiko Carstens (2):
  stackleak: allow to specify arch specific stackleak poison function
  s390/stackleak: provide fast __stackleak_poison() implementation

 arch/s390/include/asm/processor.h | 35 +++++++++++++++++++++++++++++++
 kernel/stackleak.c                | 17 +++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)

-- 
2.37.2

[PATCH 1/2] stackleak: allow to specify arch specific stackleak poison function

[Heiko Carstens]

Factor out the code that fills the stack with the stackleak poison value
in order to allow architectures to provide a faster implementation.

Acked-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 kernel/stackleak.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index c2c33d2202e9..34c9d81eea94 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -70,6 +70,18 @@ late_initcall(stackleak_sysctls_init);
 #define skip_erasing()	false
 #endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */
 
+#ifndef __stackleak_poison
+static __always_inline void __stackleak_poison(unsigned long erase_low,
+					       unsigned long erase_high,
+					       unsigned long poison)
+{
+	while (erase_low < erase_high) {
+		*(unsigned long *)erase_low = poison;
+		erase_low += sizeof(unsigned long);
+	}
+}
+#endif
+
 static __always_inline void __stackleak_erase(bool on_task_stack)
 {
 	const unsigned long task_stack_low = stackleak_task_low_bound(current);
@@ -101,10 +113,7 @@ static __always_inline void __stackleak_erase(bool on_task_stack)
 	else
 		erase_high = task_stack_high;
 
-	while (erase_low < erase_high) {
-		*(unsigned long *)erase_low = STACKLEAK_POISON;
-		erase_low += sizeof(unsigned long);
-	}
+	__stackleak_poison(erase_low, erase_high, STACKLEAK_POISON);
 
 	/* Reset the 'lowest_stack' value for the next syscall */
 	current->lowest_stack = task_stack_high;
-- 
2.37.2

Re: [PATCH 1/2] stackleak: allow to specify arch specific stackleak poison function

[Mark Rutland]

On Wed, Apr 05, 2023 at 03:08:40PM +0200, Heiko Carstens wrote:
> Factor out the code that fills the stack with the stackleak poison value
> in order to allow architectures to provide a faster implementation.
> 
> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

As on patch 2, it might be nicer to have a noinstr-safe memset64() and use that
directly, but I don't have strong feelings either way, and I'll defer to Kees's
judgement:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  kernel/stackleak.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/stackleak.c b/kernel/stackleak.c
> index c2c33d2202e9..34c9d81eea94 100644
> --- a/kernel/stackleak.c
> +++ b/kernel/stackleak.c
> @@ -70,6 +70,18 @@ late_initcall(stackleak_sysctls_init);
>  #define skip_erasing()	false
>  #endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */
>  
> +#ifndef __stackleak_poison
> +static __always_inline void __stackleak_poison(unsigned long erase_low,
> +					       unsigned long erase_high,
> +					       unsigned long poison)
> +{
> +	while (erase_low < erase_high) {
> +		*(unsigned long *)erase_low = poison;
> +		erase_low += sizeof(unsigned long);
> +	}
> +}
> +#endif
> +
>  static __always_inline void __stackleak_erase(bool on_task_stack)
>  {
>  	const unsigned long task_stack_low = stackleak_task_low_bound(current);
> @@ -101,10 +113,7 @@ static __always_inline void __stackleak_erase(bool on_task_stack)
>  	else
>  		erase_high = task_stack_high;
>  
> -	while (erase_low < erase_high) {
> -		*(unsigned long *)erase_low = STACKLEAK_POISON;
> -		erase_low += sizeof(unsigned long);
> -	}
> +	__stackleak_poison(erase_low, erase_high, STACKLEAK_POISON);
>  
>  	/* Reset the 'lowest_stack' value for the next syscall */
>  	current->lowest_stack = task_stack_high;
> -- 
> 2.37.2
> 

Re: [PATCH 1/2] stackleak: allow to specify arch specific stackleak poison function

[Heiko Carstens]

On Wed, Apr 12, 2023 at 10:03:46AM +0100, Mark Rutland wrote:
> On Wed, Apr 05, 2023 at 03:08:40PM +0200, Heiko Carstens wrote:
> > Factor out the code that fills the stack with the stackleak poison value
> > in order to allow architectures to provide a faster implementation.
> > 
> > Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> 
> As on patch 2, it might be nicer to have a noinstr-safe memset64() and use that
> directly, but I don't have strong feelings either way, and I'll defer to Kees's
> judgement:

Wouldn't that enforce that memset64() wouldn't be allowed to have an own
stackframe, since otherwise it would write poison values to it, since we
have

	if (on_task_stack)
		erase_high = current_stack_pointer;

in __stackleak_erase()?

That was actually my motiviation to make this s390 optimization an always
inline asm.

Besides that this wouldn't be a problem for at least s390, since memset64()
is an asm function which comes whithout the need for a stackframe, but on
the other hand this would add a quite subtle requirement to memset64(), if
I'm not mistaken.

Re: [PATCH 1/2] stackleak: allow to specify arch specific stackleak poison function

[Mark Rutland]

On Wed, Apr 12, 2023 at 11:58:07AM +0200, Heiko Carstens wrote:
> On Wed, Apr 12, 2023 at 10:03:46AM +0100, Mark Rutland wrote:
> > On Wed, Apr 05, 2023 at 03:08:40PM +0200, Heiko Carstens wrote:
> > > Factor out the code that fills the stack with the stackleak poison value
> > > in order to allow architectures to provide a faster implementation.
> > > 
> > > Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> > > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> > 
> > As on patch 2, it might be nicer to have a noinstr-safe memset64() and use that
> > directly, but I don't have strong feelings either way, and I'll defer to Kees's
> > judgement:
> 
> Wouldn't that enforce that memset64() wouldn't be allowed to have an own
> stackframe, since otherwise it would write poison values to it, since we
> have
> 
> 	if (on_task_stack)
> 		erase_high = current_stack_pointer;
> 
> in __stackleak_erase()?

Yes, sorry -- I was implicitly assuming that a noinstr-safe version would be
__always_inline.

> That was actually my motiviation to make this s390 optimization an always
> inline asm.
> 
> Besides that this wouldn't be a problem for at least s390, since memset64()
> is an asm function which comes whithout the need for a stackframe, but on
> the other hand this would add a quite subtle requirement to memset64(), if
> I'm not mistaken.

That's a fair enough justification, I think. Thanks for the details!

Thanks,
Mark.

[PATCH 2/2] s390/stackleak: provide fast __stackleak_poison() implementation

[Heiko Carstens]

Provide an s390 specific __stackleak_poison() implementation which is
faster than the generic variant.

For the original implementation with an enforced 4kb stackframe for the
getpid() system call the system call overhead increases by a factor of 3 if
the stackleak feature is enabled. Using the s390 mvc based variant this is
reduced to an increase of 25% instead.

This is within the expected area, since the mvc based implementation is
more or less a memset64() variant which comes with similar results. See
commit 0b77d6701cf8 ("s390: implement memset16, memset32 & memset64").

Reviewed-by: Vasily Gorbik <gor@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/include/asm/processor.h | 35 +++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index efffc28cbad8..dc17896a001a 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -118,6 +118,41 @@ unsigned long vdso_size(void);
 
 #define HAVE_ARCH_PICK_MMAP_LAYOUT
 
+#define __stackleak_poison __stackleak_poison
+static __always_inline void __stackleak_poison(unsigned long erase_low,
+					       unsigned long erase_high,
+					       unsigned long poison)
+{
+	unsigned long tmp, count;
+
+	count = erase_high - erase_low;
+	if (!count)
+		return;
+	asm volatile(
+		"	cghi	%[count],8\n"
+		"	je	2f\n"
+		"	aghi	%[count],-(8+1)\n"
+		"	srlg	%[tmp],%[count],8\n"
+		"	ltgr	%[tmp],%[tmp]\n"
+		"	jz	1f\n"
+		"0:	stg	%[poison],0(%[addr])\n"
+		"	mvc	8(256-8,%[addr]),0(%[addr])\n"
+		"	la	%[addr],256(%[addr])\n"
+		"	brctg	%[tmp],0b\n"
+		"1:	stg	%[poison],0(%[addr])\n"
+		"	larl	%[tmp],3f\n"
+		"	ex	%[count],0(%[tmp])\n"
+		"	j	4f\n"
+		"2:	stg	%[poison],0(%[addr])\n"
+		"	j	4f\n"
+		"3:	mvc	8(1,%[addr]),0(%[addr])\n"
+		"4:\n"
+		: [addr] "+&a" (erase_low), [count] "+&d" (count), [tmp] "=&a" (tmp)
+		: [poison] "d" (poison)
+		: "memory", "cc"
+		);
+}
+
 /*
  * Thread structure
  */
-- 
2.37.2

Re: [PATCH 2/2] s390/stackleak: provide fast __stackleak_poison() implementation

[Mark Rutland]

On Wed, Apr 05, 2023 at 03:08:41PM +0200, Heiko Carstens wrote:
> Provide an s390 specific __stackleak_poison() implementation which is
> faster than the generic variant.
> 
> For the original implementation with an enforced 4kb stackframe for the
> getpid() system call the system call overhead increases by a factor of 3 if
> the stackleak feature is enabled. Using the s390 mvc based variant this is
> reduced to an increase of 25% instead.
> 
> This is within the expected area, since the mvc based implementation is
> more or less a memset64() variant which comes with similar results. See
> commit 0b77d6701cf8 ("s390: implement memset16, memset32 & memset64").

With that in mind, could we use memset64() directly (if we made it
noninstr-safe)?

Mark.

> 
> Reviewed-by: Vasily Gorbik <gor@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/include/asm/processor.h | 35 +++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> index efffc28cbad8..dc17896a001a 100644
> --- a/arch/s390/include/asm/processor.h
> +++ b/arch/s390/include/asm/processor.h
> @@ -118,6 +118,41 @@ unsigned long vdso_size(void);
>  
>  #define HAVE_ARCH_PICK_MMAP_LAYOUT
>  
> +#define __stackleak_poison __stackleak_poison
> +static __always_inline void __stackleak_poison(unsigned long erase_low,
> +					       unsigned long erase_high,
> +					       unsigned long poison)
> +{
> +	unsigned long tmp, count;
> +
> +	count = erase_high - erase_low;
> +	if (!count)
> +		return;
> +	asm volatile(
> +		"	cghi	%[count],8\n"
> +		"	je	2f\n"
> +		"	aghi	%[count],-(8+1)\n"
> +		"	srlg	%[tmp],%[count],8\n"
> +		"	ltgr	%[tmp],%[tmp]\n"
> +		"	jz	1f\n"
> +		"0:	stg	%[poison],0(%[addr])\n"
> +		"	mvc	8(256-8,%[addr]),0(%[addr])\n"
> +		"	la	%[addr],256(%[addr])\n"
> +		"	brctg	%[tmp],0b\n"
> +		"1:	stg	%[poison],0(%[addr])\n"
> +		"	larl	%[tmp],3f\n"
> +		"	ex	%[count],0(%[tmp])\n"
> +		"	j	4f\n"
> +		"2:	stg	%[poison],0(%[addr])\n"
> +		"	j	4f\n"
> +		"3:	mvc	8(1,%[addr]),0(%[addr])\n"
> +		"4:\n"
> +		: [addr] "+&a" (erase_low), [count] "+&d" (count), [tmp] "=&a" (tmp)
> +		: [poison] "d" (poison)
> +		: "memory", "cc"
> +		);
> +}
> +
>  /*
>   * Thread structure
>   */
> -- 
> 2.37.2
> 

Re: [PATCH 0/2] stackleak: allow to specify arch specific stackleak poison function

[Heiko Carstens]

On Wed, Apr 05, 2023 at 03:08:39PM +0200, Heiko Carstens wrote:
> Factor out the code that fills the stack with the stackleak poison value in
> order to allow architectures to provide a faster implementation.
> 
> Use this to provide an s390 specific implementation which can fill the
> stack with the poison value much faster (factor of ~10 compared to the
> current version).
> 
> Note that the s390 stackleak support is currently only available via
> linux-next (as of today), and the s390 kernel tree at kernel.org[1].
> Therefore, if there are no objections, I'd like to add these two patches to
> the s390 tree, so they can go upstream via the next merge window together
> with the s390 support.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features&id=b94c0ebb1ec752016a3e41bfb66bb51ea905e533
> 
> Thanks,
> Heiko
> 
> Heiko Carstens (2):
>   stackleak: allow to specify arch specific stackleak poison function
>   s390/stackleak: provide fast __stackleak_poison() implementation
> 
>  arch/s390/include/asm/processor.h | 35 +++++++++++++++++++++++++++++++
>  kernel/stackleak.c                | 17 +++++++++++----
>  2 files changed, 48 insertions(+), 4 deletions(-)

Given that this series seems to be straight forward, and Mark already gave
his Ack we're going to put these two patches on the s390 git tree, even
though there was no response from Kees yet.

If there will be any complaints I'm sure we can easily solve that.