[PATCH] tools: bpf: Disable stack protector

[PATCH] tools: bpf: Disable stack protector

[Peter Foley]

Avoid build errors on distros that force the stack protector on by
default.
e.g.
  CLANG   /home/peter/linux/work/tools/bpf/bpftool/pid_iter.bpf.o
skeleton/pid_iter.bpf.c:53:5: error: A call to built-in function '__stack_chk_fail' is not supported.
int iter(struct bpf_iter__task_file *ctx)
    ^
1 error generated.

Signed-off-by: Peter Foley <pefoley2@pefoley.com>
---
 tools/bpf/bpftool/Makefile    | 1 +
 tools/bpf/runqslower/Makefile | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index f610e184ce02a..36ac0002e386f 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -215,6 +215,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
 		-I$(or $(OUTPUT),.) \
 		-I$(srctree)/tools/include/uapi/ \
 		-I$(LIBBPF_BOOTSTRAP_INCLUDE) \
+		-fno-stack-protector \
 		-g -O2 -Wall -target bpf -c $< -o $@
 	$(Q)$(LLVM_STRIP) -g $@
 
diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index 8b3d87b82b7a2..f7313cc966a04 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -60,8 +60,9 @@ $(OUTPUT)/%.skel.h: $(OUTPUT)/%.bpf.o | $(BPFTOOL)
 	$(QUIET_GEN)$(BPFTOOL) gen skeleton $< > $@
 
 $(OUTPUT)/%.bpf.o: %.bpf.c $(BPFOBJ) | $(OUTPUT)
-	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		      \
-		 -c $(filter %.c,$^) -o $@ &&				      \
+	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		\
+		 -fno-stack-protector 					\
+		 -c $(filter %.c,$^) -o $@ &&				\
 	$(LLVM_STRIP) -g $@
 
 $(OUTPUT)/%.o: %.c | $(OUTPUT)

---
base-commit: 97ec4d559d939743e8af83628be5af8da610d9dc
change-id: 20230114-bpf-918ae127b77a

Best regards,
-- 
Peter Foley <pefoley2@pefoley.com>

Re: [PATCH] tools: bpf: Disable stack protector

[Quentin Monnet]

2023-01-14 18:00 UTC-0500 ~ Peter Foley <pefoley2@pefoley.com>
> Avoid build errors on distros that force the stack protector on by
> default.
> e.g.
>   CLANG   /home/peter/linux/work/tools/bpf/bpftool/pid_iter.bpf.o
> skeleton/pid_iter.bpf.c:53:5: error: A call to built-in function '__stack_chk_fail' is not supported.
> int iter(struct bpf_iter__task_file *ctx)
>     ^
> 1 error generated.
> 
> Signed-off-by: Peter Foley <pefoley2@pefoley.com>
> ---
>  tools/bpf/bpftool/Makefile    | 1 +
>  tools/bpf/runqslower/Makefile | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index f610e184ce02a..36ac0002e386f 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -215,6 +215,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
>  		-I$(or $(OUTPUT),.) \
>  		-I$(srctree)/tools/include/uapi/ \
>  		-I$(LIBBPF_BOOTSTRAP_INCLUDE) \
> +		-fno-stack-protector \
>  		-g -O2 -Wall -target bpf -c $< -o $@
>  	$(Q)$(LLVM_STRIP) -g $@
>  

For bpftool, a similar patch was already submitted and merged to the
bpf-next tree last Friday: 878625e1c7a1 ("bpftool: Always disable stack
protection for BPF objects").

> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> index 8b3d87b82b7a2..f7313cc966a04 100644
> --- a/tools/bpf/runqslower/Makefile
> +++ b/tools/bpf/runqslower/Makefile
> @@ -60,8 +60,9 @@ $(OUTPUT)/%.skel.h: $(OUTPUT)/%.bpf.o | $(BPFTOOL)
>  	$(QUIET_GEN)$(BPFTOOL) gen skeleton $< > $@
>  
>  $(OUTPUT)/%.bpf.o: %.bpf.c $(BPFOBJ) | $(OUTPUT)
> -	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		      \
> -		 -c $(filter %.c,$^) -o $@ &&				      \
> +	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		\
> +		 -fno-stack-protector 					\
> +		 -c $(filter %.c,$^) -o $@ &&				\
>  	$(LLVM_STRIP) -g $@
>  
>  $(OUTPUT)/%.o: %.c | $(OUTPUT)

This one looks good, thanks!

I note a few more places in the repository where we compile to BPF using
clang. Given that there have been patches to add -fno-stack-protector at
several locations already, have you checked if any of these also need
the flag, by any chance, so we could fix this once and for all?

$ git grep -l 'target bpf ' | egrep -v '(Documentation|bpftool)'
kernel/bpf/preload/iterators/Makefile
samples/bpf/Makefile
samples/bpf/test_lwt_bpf.sh
tools/bpf/runqslower/Makefile
tools/build/feature/Makefile
tools/perf/Makefile.perf
tools/perf/util/llvm-utils.c
tools/testing/selftests/bpf/Makefile
tools/testing/selftests/net/bpf/Makefile
tools/testing/selftests/tc-testing/Makefile

Re: [PATCH] tools: bpf: Disable stack protector

[Eduard Zingerman]

On Mon, 2023-01-16 at 10:30 +0000, Quentin Monnet wrote:
> 2023-01-14 18:00 UTC-0500 ~ Peter Foley <pefoley2@pefoley.com>
> > Avoid build errors on distros that force the stack protector on by
> > default.
> > e.g.
> >   CLANG   /home/peter/linux/work/tools/bpf/bpftool/pid_iter.bpf.o
> > skeleton/pid_iter.bpf.c:53:5: error: A call to built-in function '__stack_chk_fail' is not supported.
> > int iter(struct bpf_iter__task_file *ctx)
> >     ^
> > 1 error generated.
> > 
> > Signed-off-by: Peter Foley <pefoley2@pefoley.com>
> > ---
> >  tools/bpf/bpftool/Makefile    | 1 +
> >  tools/bpf/runqslower/Makefile | 5 +++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index f610e184ce02a..36ac0002e386f 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -215,6 +215,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
> >  		-I$(or $(OUTPUT),.) \
> >  		-I$(srctree)/tools/include/uapi/ \
> >  		-I$(LIBBPF_BOOTSTRAP_INCLUDE) \
> > +		-fno-stack-protector \
> >  		-g -O2 -Wall -target bpf -c $< -o $@
> >  	$(Q)$(LLVM_STRIP) -g $@
> >  
> 
> For bpftool, a similar patch was already submitted and merged to the
> bpf-next tree last Friday: 878625e1c7a1 ("bpftool: Always disable stack
> protection for BPF objects").
> 
> > diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> > index 8b3d87b82b7a2..f7313cc966a04 100644
> > --- a/tools/bpf/runqslower/Makefile
> > +++ b/tools/bpf/runqslower/Makefile
> > @@ -60,8 +60,9 @@ $(OUTPUT)/%.skel.h: $(OUTPUT)/%.bpf.o | $(BPFTOOL)
> >  	$(QUIET_GEN)$(BPFTOOL) gen skeleton $< > $@
> >  
> >  $(OUTPUT)/%.bpf.o: %.bpf.c $(BPFOBJ) | $(OUTPUT)
> > -	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		      \
> > -		 -c $(filter %.c,$^) -o $@ &&				      \
> > +	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		\
> > +		 -fno-stack-protector 					\
> > +		 -c $(filter %.c,$^) -o $@ &&				\
> >  	$(LLVM_STRIP) -g $@
> >  
> >  $(OUTPUT)/%.o: %.c | $(OUTPUT)
> 
> This one looks good, thanks!
> 
> I note a few more places in the repository where we compile to BPF using
> clang. Given that there have been patches to add -fno-stack-protector at
> several locations already, have you checked if any of these also need
> the flag, by any chance, so we could fix this once and for all?
> 
> $ git grep -l 'target bpf ' | egrep -v '(Documentation|bpftool)'
> kernel/bpf/preload/iterators/Makefile
> samples/bpf/Makefile
> samples/bpf/test_lwt_bpf.sh
> tools/bpf/runqslower/Makefile
> tools/build/feature/Makefile
> tools/perf/Makefile.perf
> tools/perf/util/llvm-utils.c
> tools/testing/selftests/bpf/Makefile
> tools/testing/selftests/net/bpf/Makefile
> tools/testing/selftests/tc-testing/Makefile

A bit tangential, but since BPF LLVM backend does not support the
stack protector (should it?) there is also an option to adjust LLVM
to avoid this instrumentation, WDYT?

Re: [PATCH] tools: bpf: Disable stack protector

[Peter Foley]

On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> A bit tangential, but since BPF LLVM backend does not support the
> stack protector (should it?) there is also an option to adjust LLVM
> to avoid this instrumentation, WDYT?
>

That would probably be worth doing, yes.
But given that won't help already released versions of clang, it
should probably happen in addition to this patch.

Re: [PATCH] tools: bpf: Disable stack protector

[Eduard Zingerman]

On Mon, 2023-01-16 at 14:49 -0800, Peter Foley wrote:
> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > A bit tangential, but since BPF LLVM backend does not support the
> > stack protector (should it?) there is also an option to adjust LLVM
> > to avoid this instrumentation, WDYT?
> > 
> 
> That would probably be worth doing, yes.

Ok, thanks for the input, I'll see what can be done.

> But given that won't help already released versions of clang, it
> should probably happen in addition to this patch.

Yes, of-course.

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/16/23 2:49 PM, Peter Foley wrote:
> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>
>> A bit tangential, but since BPF LLVM backend does not support the
>> stack protector (should it?) there is also an option to adjust LLVM
>> to avoid this instrumentation, WDYT?
>>
> 
> That would probably be worth doing, yes.
> But given that won't help already released versions of clang, it
> should probably happen in addition to this patch.

Peter,

If I understand correctly (by inspecting clang code), the stack 
protector is off by default. Do you have link to Gentoo build
page to show how they enable stack protector? cmake config or
a private patch?

Jose,

How gcc-bpf handle stack protector? The compiler just disables
stack protector for bpf target?

Re: [PATCH] tools: bpf: Disable stack protector

[Peter Foley]

On Mon, Jan 16, 2023 at 11:05 PM Yonghong Song <yhs@meta.com> wrote:
>
> If I understand correctly (by inspecting clang code), the stack
> protector is off by default. Do you have link to Gentoo build
> page to show how they enable stack protector? cmake config or
> a private patch?
>
The relevant override appears to be
https://github.com/gentoo/gentoo/blob/c5247250e9d4a09e67a602965a5f72be3cebbf34/sys-devel/clang-common/clang-common-15.0.7.ebuild#L93

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/16/23 11:09 PM, Peter Foley wrote:
> On Mon, Jan 16, 2023 at 11:05 PM Yonghong Song <yhs@meta.com> wrote:
>>
>> If I understand correctly (by inspecting clang code), the stack
>> protector is off by default. Do you have link to Gentoo build
>> page to show how they enable stack protector? cmake config or
>> a private patch?
>>
> The relevant override appears to be
> https://github.com/gentoo/gentoo/blob/c5247250e9d4a09e67a602965a5f72be3cebbf34/sys-devel/clang-common/clang-common-15.0.7.ebuild#L93

Thanks for the link. Looks like this is a security feature added by 
hardened_gentoo progject (https://wiki.gentoo.org/wiki/Hardened_Gentoo)
which unconditionally added -fstack-protector-strong to the clang
compilation flag.

Re: [PATCH] tools: bpf: Disable stack protector

[Jose E. Marchesi]

> On 1/16/23 2:49 PM, Peter Foley wrote:
>> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>
>>> A bit tangential, but since BPF LLVM backend does not support the
>>> stack protector (should it?) there is also an option to adjust LLVM
>>> to avoid this instrumentation, WDYT?
>>>
>> That would probably be worth doing, yes.
>> But given that won't help already released versions of clang, it
>> should probably happen in addition to this patch.
>
> Peter,
>
> If I understand correctly (by inspecting clang code), the stack
> protector is off by default. Do you have link to Gentoo build
> page to show how they enable stack protector? cmake config or
> a private patch?
>
> Jose,
>
> How gcc-bpf handle stack protector? The compiler just disables
> stack protector for bpf target?

It doesn't.  -fstack-protector is disabled by default in GCC.  When you
use it you get something like:

  $ echo 'int foo() { char s[256]; return s[3]; }' | bpf-unknown-none-gcc \
    -fstack-protector -S -o foo.s -O2 -xc -
  $ cat foo.s
  	.file	"<stdin>"
  	.text
  	.align	3
  	.global	foo
  	.type	foo, @function
  foo:
  	lddw	%r1,__stack_chk_guard
  	ldxdw	%r0,[%r1+0]
  	stxdw	[%fp+-8],%r0
  	ldxb	%r0,[%fp+-261]
  	lsh	%r0,56
  	arsh	%r0,56
  	ldxdw	%r2,[%fp+-8]
  	ldxdw	%r3,[%r1+0]
 	jne	%r2,%r3,.L4
  	exit
  .L4:
  	call	__stack_chk_fail
  	.size	foo, .-foo
  	.ident	"GCC: (GNU) 12.0.0 20211206 (experimental)"

i.e. it pushes a stack canary and checks it upon function exit, calling
__stack_chk_fail.

If clang has -fstack-protector ON by default and you change the BPF
backend in order to ignore the flag, I think we should do the same in
GCC.

Re: [PATCH] tools: bpf: Disable stack protector

[Jose E. Marchesi]

>> On 1/16/23 2:49 PM, Peter Foley wrote:
>>> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>
>>>> A bit tangential, but since BPF LLVM backend does not support the
>>>> stack protector (should it?) there is also an option to adjust LLVM
>>>> to avoid this instrumentation, WDYT?
>>>>
>>> That would probably be worth doing, yes.
>>> But given that won't help already released versions of clang, it
>>> should probably happen in addition to this patch.
>>
>> Peter,
>>
>> If I understand correctly (by inspecting clang code), the stack
>> protector is off by default. Do you have link to Gentoo build
>> page to show how they enable stack protector? cmake config or
>> a private patch?
>>
>> Jose,
>>
>> How gcc-bpf handle stack protector? The compiler just disables
>> stack protector for bpf target?
>
> It doesn't.  -fstack-protector is disabled by default in GCC.  When you
> use it you get something like:
>
>   $ echo 'int foo() { char s[256]; return s[3]; }' | bpf-unknown-none-gcc \
>     -fstack-protector -S -o foo.s -O2 -xc -
>   $ cat foo.s
>   	.file	"<stdin>"
>   	.text
>   	.align	3
>   	.global	foo
>   	.type	foo, @function
>   foo:
>   	lddw	%r1,__stack_chk_guard
>   	ldxdw	%r0,[%r1+0]
>   	stxdw	[%fp+-8],%r0
>   	ldxb	%r0,[%fp+-261]
>   	lsh	%r0,56
>   	arsh	%r0,56
>   	ldxdw	%r2,[%fp+-8]
>   	ldxdw	%r3,[%r1+0]
>  	jne	%r2,%r3,.L4
>   	exit
>   .L4:
>   	call	__stack_chk_fail
>   	.size	foo, .-foo
>   	.ident	"GCC: (GNU) 12.0.0 20211206 (experimental)"
>
> i.e. it pushes a stack canary and checks it upon function exit, calling
> __stack_chk_fail.
>
> If clang has -fstack-protector ON by default and you change the BPF
> backend in order to ignore the flag, I think we should do the same in
> GCC.

I went ahead and pushed the patch below to GCC master.  If
-fstack-protector is ever considered useful in the architecture, we can
always stop disabling it.

I would recommend to change the default for -fstack-protector in clang
to be off by default when targetting BPF targets, and to emit the same
or similar note to the user when the option is enabled explicitly with
-fstack-protector:

  note: ‘-fstack-protector’ does not work  on this architecture

WDYT?

From 3b81f5c4d8e0d79cbd6927d004185707c14e54b2 Mon Sep 17 00:00:00 2001
Date: Tue, 17 Jan 2023 17:16:32 +0100
Subject: [COMMITTED] bpf: disable -fstack-protector in BPF

The stack protector is not supported in BPF.  This patch disables
-fstack-protector in bpf-* targets, along with the emission of a note
indicating that the feature is not supported in this platform.

Regtested in bpf-unknown-none.

gcc/ChangeLog:

	* config/bpf/bpf.cc (bpf_option_override): Disable
	-fstack-protector.
---
 gcc/config/bpf/bpf.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
index 576a1fe8eab..b268801d00c 100644
--- a/gcc/config/bpf/bpf.cc
+++ b/gcc/config/bpf/bpf.cc
@@ -253,6 +253,14 @@ bpf_option_override (void)
   if (bpf_has_jmp32 == -1)
     bpf_has_jmp32 = (bpf_isa >= ISA_V3);
 
+  /* Disable -fstack-protector as it is not supported in BPF.  */
+  if (flag_stack_protect)
+    {
+      inform (input_location,
+              "%<-fstack-protector%> does not work "
+              " on this architecture");
+      flag_stack_protect = 0;
+    }
 }
 
 #undef TARGET_OPTION_OVERRIDE
-- 
2.30.2

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/17/23 8:31 AM, Jose E. Marchesi wrote:
> 
>>> On 1/16/23 2:49 PM, Peter Foley wrote:
>>>> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>>
>>>>> A bit tangential, but since BPF LLVM backend does not support the
>>>>> stack protector (should it?) there is also an option to adjust LLVM
>>>>> to avoid this instrumentation, WDYT?
>>>>>
>>>> That would probably be worth doing, yes.
>>>> But given that won't help already released versions of clang, it
>>>> should probably happen in addition to this patch.
>>>
>>> Peter,
>>>
>>> If I understand correctly (by inspecting clang code), the stack
>>> protector is off by default. Do you have link to Gentoo build
>>> page to show how they enable stack protector? cmake config or
>>> a private patch?
>>>
>>> Jose,
>>>
>>> How gcc-bpf handle stack protector? The compiler just disables
>>> stack protector for bpf target?
>>
>> It doesn't.  -fstack-protector is disabled by default in GCC.  When you
>> use it you get something like:
>>
>>    $ echo 'int foo() { char s[256]; return s[3]; }' | bpf-unknown-none-gcc \
>>      -fstack-protector -S -o foo.s -O2 -xc -
>>    $ cat foo.s
>>    	.file	"<stdin>"
>>    	.text
>>    	.align	3
>>    	.global	foo
>>    	.type	foo, @function
>>    foo:
>>    	lddw	%r1,__stack_chk_guard
>>    	ldxdw	%r0,[%r1+0]
>>    	stxdw	[%fp+-8],%r0
>>    	ldxb	%r0,[%fp+-261]
>>    	lsh	%r0,56
>>    	arsh	%r0,56
>>    	ldxdw	%r2,[%fp+-8]
>>    	ldxdw	%r3,[%r1+0]
>>   	jne	%r2,%r3,.L4
>>    	exit
>>    .L4:
>>    	call	__stack_chk_fail
>>    	.size	foo, .-foo
>>    	.ident	"GCC: (GNU) 12.0.0 20211206 (experimental)"
>>
>> i.e. it pushes a stack canary and checks it upon function exit, calling
>> __stack_chk_fail.
>>
>> If clang has -fstack-protector ON by default and you change the BPF
>> backend in order to ignore the flag, I think we should do the same in
>> GCC.
> 
> I went ahead and pushed the patch below to GCC master.  If
> -fstack-protector is ever considered useful in the architecture, we can
> always stop disabling it.
> 
> I would recommend to change the default for -fstack-protector in clang
> to be off by default when targetting BPF targets, and to emit the same
> or similar note to the user when the option is enabled explicitly with
> -fstack-protector:
> 
>    note: ‘-fstack-protector’ does not work  on this architecture
> 
> WDYT?
> 
>  From 3b81f5c4d8e0d79cbd6927d004185707c14e54b2 Mon Sep 17 00:00:00 2001
> Date: Tue, 17 Jan 2023 17:16:32 +0100
> Subject: [COMMITTED] bpf: disable -fstack-protector in BPF
> 
> The stack protector is not supported in BPF.  This patch disables
> -fstack-protector in bpf-* targets, along with the emission of a note
> indicating that the feature is not supported in this platform.
> 
> Regtested in bpf-unknown-none.
> 
> gcc/ChangeLog:
> 
> 	* config/bpf/bpf.cc (bpf_option_override): Disable
> 	-fstack-protector.
> ---
>   gcc/config/bpf/bpf.cc | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
> index 576a1fe8eab..b268801d00c 100644
> --- a/gcc/config/bpf/bpf.cc
> +++ b/gcc/config/bpf/bpf.cc
> @@ -253,6 +253,14 @@ bpf_option_override (void)
>     if (bpf_has_jmp32 == -1)
>       bpf_has_jmp32 = (bpf_isa >= ISA_V3);
>   
> +  /* Disable -fstack-protector as it is not supported in BPF.  */
> +  if (flag_stack_protect)
> +    {
> +      inform (input_location,
> +              "%<-fstack-protector%> does not work "
> +              " on this architecture");
> +      flag_stack_protect = 0;
> +    }
>   }

Thanks, just replied based on a previous email
communication a while back. Yes, clang could
do similar things.

>   
>   #undef TARGET_OPTION_OVERRIDE

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/17/23 5:23 AM, Jose E. Marchesi wrote:
> 
>> On 1/16/23 2:49 PM, Peter Foley wrote:
>>> On Mon, Jan 16, 2023 at 4:59 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>
>>>> A bit tangential, but since BPF LLVM backend does not support the
>>>> stack protector (should it?) there is also an option to adjust LLVM
>>>> to avoid this instrumentation, WDYT?
>>>>
>>> That would probably be worth doing, yes.
>>> But given that won't help already released versions of clang, it
>>> should probably happen in addition to this patch.
>>
>> Peter,
>>
>> If I understand correctly (by inspecting clang code), the stack
>> protector is off by default. Do you have link to Gentoo build
>> page to show how they enable stack protector? cmake config or
>> a private patch?
>>
>> Jose,
>>
>> How gcc-bpf handle stack protector? The compiler just disables
>> stack protector for bpf target?
> 
> It doesn't.  -fstack-protector is disabled by default in GCC.  When you
> use it you get something like:
> 
>    $ echo 'int foo() { char s[256]; return s[3]; }' | bpf-unknown-none-gcc \
>      -fstack-protector -S -o foo.s -O2 -xc -
>    $ cat foo.s
>    	.file	"<stdin>"
>    	.text
>    	.align	3
>    	.global	foo
>    	.type	foo, @function
>    foo:
>    	lddw	%r1,__stack_chk_guard
>    	ldxdw	%r0,[%r1+0]
>    	stxdw	[%fp+-8],%r0
>    	ldxb	%r0,[%fp+-261]
>    	lsh	%r0,56
>    	arsh	%r0,56
>    	ldxdw	%r2,[%fp+-8]
>    	ldxdw	%r3,[%r1+0]
>   	jne	%r2,%r3,.L4
>    	exit
>    .L4:
>    	call	__stack_chk_fail
>    	.size	foo, .-foo
>    	.ident	"GCC: (GNU) 12.0.0 20211206 (experimental)"
> 
> i.e. it pushes a stack canary and checks it upon function exit, calling
> __stack_chk_fail.
> 
> If clang has -fstack-protector ON by default and you change the BPF
> backend in order to ignore the flag, I think we should do the same in
> GCC.

clang itself does not have -fstack-protector on by default. It is
hardened gentoo distribution unconditionally added -fstack-protector
to its clang distribution.

In clang/lib/Driver/ToolChains/Clang.cpp, we have
    ...
   // NVPTX doesn't support stack protectors; from the compiler's 
perspective, it
   // doesn't even have a stack!
   if (EffectiveTriple.isNVPTX())
     return;

and -fstack-protector is not effective for NVPTX. I guess we
could make it noop for BPF target as well.

Re: [PATCH] tools: bpf: Disable stack protector

[Eduard Zingerman]

On Sat, 2023-01-14 at 18:00 -0500, Peter Foley wrote:
> Avoid build errors on distros that force the stack protector on by
> default.
> e.g.
>   CLANG   /home/peter/linux/work/tools/bpf/bpftool/pid_iter.bpf.o
> skeleton/pid_iter.bpf.c:53:5: error: A call to built-in function '__stack_chk_fail' is not supported.
> int iter(struct bpf_iter__task_file *ctx)
>     ^
> 1 error generated.
> 
> Signed-off-by: Peter Foley <pefoley2@pefoley.com>
> ---
>  tools/bpf/bpftool/Makefile    | 1 +
>  tools/bpf/runqslower/Makefile | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index f610e184ce02a..36ac0002e386f 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -215,6 +215,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
>  		-I$(or $(OUTPUT),.) \
>  		-I$(srctree)/tools/include/uapi/ \
>  		-I$(LIBBPF_BOOTSTRAP_INCLUDE) \
> +		-fno-stack-protector \

While working on clang patch to disable stack protector
for BPF target I've noticed that there is an option to
disable default configuration file altogether [1]:

  --no-default-config

Should we consider it instead of -fno-stack-protector
to shield ourselves from any potential distro-specific
changes?

[1] https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-no-default-config

>  		-g -O2 -Wall -target bpf -c $< -o $@
>  	$(Q)$(LLVM_STRIP) -g $@
>  
> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> index 8b3d87b82b7a2..f7313cc966a04 100644
> --- a/tools/bpf/runqslower/Makefile
> +++ b/tools/bpf/runqslower/Makefile
> @@ -60,8 +60,9 @@ $(OUTPUT)/%.skel.h: $(OUTPUT)/%.bpf.o | $(BPFTOOL)
>  	$(QUIET_GEN)$(BPFTOOL) gen skeleton $< > $@
>  
>  $(OUTPUT)/%.bpf.o: %.bpf.c $(BPFOBJ) | $(OUTPUT)
> -	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		      \
> -		 -c $(filter %.c,$^) -o $@ &&				      \
> +	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		\
> +		 -fno-stack-protector 					\
> +		 -c $(filter %.c,$^) -o $@ &&				\
>  	$(LLVM_STRIP) -g $@
>  
>  $(OUTPUT)/%.o: %.c | $(OUTPUT)
> 
> ---
> base-commit: 97ec4d559d939743e8af83628be5af8da610d9dc
> change-id: 20230114-bpf-918ae127b77a
> 
> Best regards,

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/18/23 11:28 AM, Eduard Zingerman wrote:
> On Sat, 2023-01-14 at 18:00 -0500, Peter Foley wrote:
>> Avoid build errors on distros that force the stack protector on by
>> default.
>> e.g.
>>    CLANG   /home/peter/linux/work/tools/bpf/bpftool/pid_iter.bpf.o
>> skeleton/pid_iter.bpf.c:53:5: error: A call to built-in function '__stack_chk_fail' is not supported.
>> int iter(struct bpf_iter__task_file *ctx)
>>      ^
>> 1 error generated.
>>
>> Signed-off-by: Peter Foley <pefoley2@pefoley.com>
>> ---
>>   tools/bpf/bpftool/Makefile    | 1 +
>>   tools/bpf/runqslower/Makefile | 5 +++--
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
>> index f610e184ce02a..36ac0002e386f 100644
>> --- a/tools/bpf/bpftool/Makefile
>> +++ b/tools/bpf/bpftool/Makefile
>> @@ -215,6 +215,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF_BOOTSTRAP)
>>   		-I$(or $(OUTPUT),.) \
>>   		-I$(srctree)/tools/include/uapi/ \
>>   		-I$(LIBBPF_BOOTSTRAP_INCLUDE) \
>> +		-fno-stack-protector \
> 
> While working on clang patch to disable stack protector
> for BPF target I've noticed that there is an option to
> disable default configuration file altogether [1]:
> 
>    --no-default-config
> 
> Should we consider it instead of -fno-stack-protector
> to shield ourselves from any potential distro-specific
> changes?

Peter, could you help check whether adding --no-default-config works
in your environment or not?

> 
> [1] https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-no-default-config
> 
>>   		-g -O2 -Wall -target bpf -c $< -o $@
>>   	$(Q)$(LLVM_STRIP) -g $@
>>   
>> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
>> index 8b3d87b82b7a2..f7313cc966a04 100644
>> --- a/tools/bpf/runqslower/Makefile
>> +++ b/tools/bpf/runqslower/Makefile
>> @@ -60,8 +60,9 @@ $(OUTPUT)/%.skel.h: $(OUTPUT)/%.bpf.o | $(BPFTOOL)
>>   	$(QUIET_GEN)$(BPFTOOL) gen skeleton $< > $@
>>   
>>   $(OUTPUT)/%.bpf.o: %.bpf.c $(BPFOBJ) | $(OUTPUT)
>> -	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		      \
>> -		 -c $(filter %.c,$^) -o $@ &&				      \
>> +	$(QUIET_GEN)$(CLANG) -g -O2 -target bpf $(INCLUDES)		\
>> +		 -fno-stack-protector 					\
>> +		 -c $(filter %.c,$^) -o $@ &&				\
>>   	$(LLVM_STRIP) -g $@
>>   
>>   $(OUTPUT)/%.o: %.c | $(OUTPUT)
>>
>> ---
>> base-commit: 97ec4d559d939743e8af83628be5af8da610d9dc
>> change-id: 20230114-bpf-918ae127b77a
>>
>> Best regards,
> 

Re: [PATCH] tools: bpf: Disable stack protector

[Peter Foley]

On Wed, Jan 18, 2023 at 11:34 PM Yonghong Song <yhs@meta.com> wrote:
> On 1/18/23 11:28 AM, Eduard Zingerman wrote:
> >
> > While working on clang patch to disable stack protector
> > for BPF target I've noticed that there is an option to
> > disable default configuration file altogether [1]:
> >
> >    --no-default-config
> >
> > Should we consider it instead of -fno-stack-protector
> > to shield ourselves from any potential distro-specific
> > changes?
>
> Peter, could you help check whether adding --no-default-config works
> in your environment or not?
>
> >
> > [1] https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-no-default-config

I guess I could, but I'm not convinced that's the right thing to do.
Ideally problems with distro-specific configs would cause loud
failures (like this one) and result in fixes like the changes being
made to upstream clang/gcc.
Simply unconditionally disabling distro configs seems to be the wrong
way to approach this and makes it less likely that future problems
will be reported in the first place.

Re: [PATCH] tools: bpf: Disable stack protector

[Yonghong Song]

On 1/22/23 8:28 PM, Peter Foley wrote:
> On Wed, Jan 18, 2023 at 11:34 PM Yonghong Song <yhs@meta.com> wrote:
>> On 1/18/23 11:28 AM, Eduard Zingerman wrote:
>>>
>>> While working on clang patch to disable stack protector
>>> for BPF target I've noticed that there is an option to
>>> disable default configuration file altogether [1]:
>>>
>>>     --no-default-config
>>>
>>> Should we consider it instead of -fno-stack-protector
>>> to shield ourselves from any potential distro-specific
>>> changes?
>>
>> Peter, could you help check whether adding --no-default-config works
>> in your environment or not?
>>
>>>
>>> [1] https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-no-default-config
> 
> I guess I could, but I'm not convinced that's the right thing to do.
> Ideally problems with distro-specific configs would cause loud
> failures (like this one) and result in fixes like the changes being
> made to upstream clang/gcc.
> Simply unconditionally disabling distro configs seems to be the wrong
> way to approach this and makes it less likely that future problems
> will be reported in the first place.

Thanks for confirming. Eduard has implemented a proper fix in clang
(https://reviews.llvm.org/D142046) which will warn if -fstack-protector
is enabled. In gentoo case, since -fno-stack-protector is appended to
compilation flags, no warning will be issued, so we are all good here.