Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Alexei Starovoitov]

On Thu, May 10, 2018 at 10:58 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> I see no option, but to fix the kernel.
> Regardless whether it's called user space breakage or kernel breakage.

Peter,

could you please ack the patch or better yet take it into tip tree
and send to Linus asap ?
rc5 is almost here and we didn't have full test coverage
for more than a month due to this issue.

Thanks

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Thomas Gleixner]

On Sat, 12 May 2018, Alexei Starovoitov wrote:
> On Thu, May 10, 2018 at 10:58 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > I see no option, but to fix the kernel.
> > Regardless whether it's called user space breakage or kernel breakage.

There is a big difference. If you are abusing a kernel internal header in a
user space tool, then there is absolutely ZERO excuse for requesting that
the header in question has to be modified.

But yes, the situation is slightly different here because tools which
create trace event magic _HAVE_ to pull in kernel headers. At the same time
these tools depend on a compiler which failed to implement asm_goto for
fricking 8 years.

So while Boris is right, that nothing has to fiddle with a kernel only
header, I grumpily agree with you that we need a workaround in the kernel
for this particular issue.

> could you please ack the patch or better yet take it into tip tree
> and send to Linus asap ?

Nope. The patch is a horrible hack.

Why the heck do we need that extra fugly define? That has exactly zero
value simply because we already have a define which denotes availablity of
ASM GOTO: CC_HAVE_ASM_GOTO.

In case of samples/bpf/ and libbcc the compile does not go through the
arch/x86 Makefile which stops the build anyway when ASM_GOTO is
missing. Those builds merily pull in the headers and have their own build
magic, which is broken btw: Changing a kernel header which gets pulled into
the build does not rebuild anything in samples/bpf. Qualitee..

So we can just use CC_HAVE_ASM_GOTO and be done with it.

But we also want the tools which needs this to be aware of this. Peter
requested -D __BPF__ several times which got ignored. It's not too much of
a request to add that.

Find a patch which deos exactly this for samples/bpf, but also allows other
tools to build with a warning emitted so they get fixed.

Thanks,

	tglx

8<----------------
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -140,6 +140,20 @@ extern void clear_cpu_cap(struct cpuinfo
 
 #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
 
+#ifndef CC_HAVE_ASM_GOTO
+
+/*
+ * Workaround for the sake of BPF compilation which utilizes kernel
+ * headers, but clang does not support ASM GOTO and fails the build.
+ */
+#ifndef __BPF__
+#warning "Compiler lacks ASM_GOTO support. Add -D __BPF__ to your compiler arguments"
+#endif
+
+#define static_cpu_has(bit)		boot_cpu_has(bit)
+
+#else
+
 /*
  * Static testing of CPU features.  Used the same as boot_cpu_has().
  * These will statically patch the target code for additional
@@ -195,6 +209,7 @@ static __always_inline __pure bool _stat
 		boot_cpu_has(bit) :				\
 		_static_cpu_has(bit)				\
 )
+#endif
 
 #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
 #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -255,7 +255,7 @@ verify_target_bpf: verify_cmds
 $(obj)/%.o: $(src)/%.c
 	$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
 		-I$(srctree)/tools/testing/selftests/bpf/ \
-		-D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
+		-D__KERNEL__ -D__BPF__ -Wno-unused-value -Wno-pointer-sign \
 		-D__TARGET_ARCH_$(ARCH) -Wno-compare-distinct-pointer-types \
 		-Wno-gnu-variable-sized-type-not-at-end \
 		-Wno-address-of-packed-member -Wno-tautological-compare \

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Alexei Starovoitov]

On Sat, May 12, 2018 at 10:30:02PM +0200, Thomas Gleixner wrote:
> On Sat, 12 May 2018, Alexei Starovoitov wrote:
> > On Thu, May 10, 2018 at 10:58 AM, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > I see no option, but to fix the kernel.
> > > Regardless whether it's called user space breakage or kernel breakage.
> 
> There is a big difference. If you are abusing a kernel internal header in a
> user space tool, then there is absolutely ZERO excuse for requesting that
> the header in question has to be modified.
> 
> But yes, the situation is slightly different here because tools which
> create trace event magic _HAVE_ to pull in kernel headers. At the same time
> these tools depend on a compiler which failed to implement asm_goto for
> fricking 8 years.

As a maintainer of a piece of llvm codebase I have to say that
this bullying tactic has the opposite effect.
The inline asm is processed by gcc and llvm very differently.
gcc is leaking internal backend implementation details into inline asm
syntax. It makes little sense for llvm to do the same, since compiler
codegen is completely different. gcc doesn't have integrated assembler
whereas llvm not only can parse asm, but can potentially optimize it as well.
Instead of demanding asm-goto that matches gcc one to one it's better
to work with the community to define the syntax that works for both
kernel and llvm.

> So while Boris is right, that nothing has to fiddle with a kernel only
> header, I grumpily agree with you that we need a workaround in the kernel
> for this particular issue.
> 
> > could you please ack the patch or better yet take it into tip tree
> > and send to Linus asap ?
> 
> Nope. The patch is a horrible hack.
> 
> Why the heck do we need that extra fugly define? That has exactly zero
> value simply because we already have a define which denotes availablity of
> ASM GOTO: CC_HAVE_ASM_GOTO.

I agree. That's why the v1 patch that was using CC_HAVE_ASM_GOTO was better:
https://patchwork.kernel.org/patch/10333829/
I'm fine on adding a warning to it though.

> In case of samples/bpf/ and libbcc the compile does not go through the
> arch/x86 Makefile which stops the build anyway when ASM_GOTO is
> missing. Those builds merily pull in the headers and have their own build
> magic, which is broken btw: Changing a kernel header which gets pulled into
> the build does not rebuild anything in samples/bpf. Qualitee..
> 
> So we can just use CC_HAVE_ASM_GOTO and be done with it.
> 
> But we also want the tools which needs this to be aware of this. Peter
> requested -D __BPF__ several times which got ignored. It's not too much of
> a request to add that.

quite the opposite.
It was explained already why -D__BPF__ makes little sense.
It's like saying that -D__arm__ has to be specified in command line.

clang automatically adds -D__arm__ when '-target arm' is used
and adds -D__BPF__ when '-target bpf' is used.
For samples/bpf, libbcc and other cases we have to use -target native.
If we do '-target native -D__BPF__' that's just like trying to compile
kernel headers with '-target x86 -D__arm__'. Absurd.

> Find a patch which deos exactly this for samples/bpf, but also allows other
> tools to build with a warning emitted so they get fixed.

agree

> Thanks,
> 
> 	tglx
> 
> 8<----------------
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -140,6 +140,20 @@ extern void clear_cpu_cap(struct cpuinfo
>  
>  #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
>  
> +#ifndef CC_HAVE_ASM_GOTO
> +
> +/*
> + * Workaround for the sake of BPF compilation which utilizes kernel
> + * headers, but clang does not support ASM GOTO and fails the build.
> + */
> +#ifndef __BPF__
> +#warning "Compiler lacks ASM_GOTO support. Add -D __BPF__ to your compiler arguments"
> +#endif

Agree.
The warning makes sense to me, but it has to be different macro name.
How about -D__BPF_TRACING__ or -D__BPF_KPROBES__ or something similar ?
Such name will also make it clear that only tracing bpf programs
need this. Networking programs shouldn't be including kernel headers.
There was never a need, but since the tracing progs are often used
as an example people copy paste makefiles too.
We tried to document it as much as possible, but people still use
'clang -target native -I/kernel/includes bpf_prog.c -emit-llvm | llc -march=bpf'
in their builds.
(sometimes as a workaround for setups where clang is older version,
but llc/llvm is new)
Now they will see this warning and it will force them to think whether
they actually need the kernel headers.

> +
> +#define static_cpu_has(bit)		boot_cpu_has(bit)
> +
> +#else
> +
>  /*
>   * Static testing of CPU features.  Used the same as boot_cpu_has().
>   * These will statically patch the target code for additional
> @@ -195,6 +209,7 @@ static __always_inline __pure bool _stat
>  		boot_cpu_has(bit) :				\
>  		_static_cpu_has(bit)				\
>  )
> +#endif
>  
>  #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
>  #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -255,7 +255,7 @@ verify_target_bpf: verify_cmds
>  $(obj)/%.o: $(src)/%.c
>  	$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
>  		-I$(srctree)/tools/testing/selftests/bpf/ \
> -		-D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
> +		-D__KERNEL__ -D__BPF__ -Wno-unused-value -Wno-pointer-sign \

Yep. In samples/bpf and libbcc we can selectively add -D__BPF_TRACING__
I think sysdig and other folks can live with that as well.
Agree?

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Thomas Gleixner]

On Sun, 13 May 2018, Alexei Starovoitov wrote:
> On Sat, May 12, 2018 at 10:30:02PM +0200, Thomas Gleixner wrote:
> > But yes, the situation is slightly different here because tools which
> > create trace event magic _HAVE_ to pull in kernel headers. At the same time
> > these tools depend on a compiler which failed to implement asm_goto for
> > fricking 8 years.
> 
> As a maintainer of a piece of llvm codebase I have to say that
> this bullying tactic has the opposite effect.

I'm not bullying at all. Its a fact that the discussion about asm goto is
dragging out 8 years now. We've stayed away from mandating it for quite
some time, but at some point it just doesn't make sense anymore.

> The inline asm is processed by gcc and llvm very differently.  gcc is
> leaking internal backend implementation details into inline asm
> syntax. It makes little sense for llvm to do the same, since compiler
> codegen is completely different. gcc doesn't have integrated assembler
> whereas llvm not only can parse asm, but can potentially optimize it as
> well.  Instead of demanding asm-goto that matches gcc one to one it's
> better to work with the community to define the syntax that works for
> both kernel and llvm.

Come on, we surely are open for discussions, but what I've seen so far is
just 'oh we can't do this because' instead of a sane proposal how it can be
done w/o rewriting the whole ASM GOTO stuff in the kernel or even
duplicating it.

> > + * Workaround for the sake of BPF compilation which utilizes kernel
> > + * headers, but clang does not support ASM GOTO and fails the build.
> > + */
> > +#ifndef __BPF__
> > +#warning "Compiler lacks ASM_GOTO support. Add -D __BPF__ to your compiler arguments"
> > +#endif
> 
> Agree.
> The warning makes sense to me, but it has to be different macro name.
> How about -D__BPF_TRACING__ or -D__BPF_KPROBES__ or something similar ?

Fair enough.

> Such name will also make it clear that only tracing bpf programs
> need this. Networking programs shouldn't be including kernel headers.
> There was never a need, but since the tracing progs are often used
> as an example people copy paste makefiles too.
> We tried to document it as much as possible, but people still use
> 'clang -target native -I/kernel/includes bpf_prog.c -emit-llvm | llc -march=bpf'
> in their builds.
> (sometimes as a workaround for setups where clang is older version,
> but llc/llvm is new)
> Now they will see this warning and it will force them to think whether
> they actually need the kernel headers.

Makes sense.

> > +
> > +#define static_cpu_has(bit)		boot_cpu_has(bit)
> > +
> > +#else
> > +
> >  /*
> >   * Static testing of CPU features.  Used the same as boot_cpu_has().
> >   * These will statically patch the target code for additional
> > @@ -195,6 +209,7 @@ static __always_inline __pure bool _stat
> >  		boot_cpu_has(bit) :				\
> >  		_static_cpu_has(bit)				\
> >  )
> > +#endif
> >  
> >  #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
> >  #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
> > --- a/samples/bpf/Makefile
> > +++ b/samples/bpf/Makefile
> > @@ -255,7 +255,7 @@ verify_target_bpf: verify_cmds
> >  $(obj)/%.o: $(src)/%.c
> >  	$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
> >  		-I$(srctree)/tools/testing/selftests/bpf/ \
> > -		-D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
> > +		-D__KERNEL__ -D__BPF__ -Wno-unused-value -Wno-pointer-sign \
> 
> Yep. In samples/bpf and libbcc we can selectively add -D__BPF_TRACING__
> I think sysdig and other folks can live with that as well.
> Agree?

Sure. Care to send an updated patch?

Thanks,

	tglx

[PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Yonghong Song]

Commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
removed X86_FAST_FEATURE_TESTS and make macro static_cpu_has() always
use __always_inline function _static_cpu_has() funciton.
The static_cpu_has() uses gcc feature asm goto construct,
which is not supported by clang.

Issues
======

Currently, for BPF programs written in C, the only widely-supported
compiler is clang. Because of clang lacking support
of asm goto construct, if you try to compile
bpf programs under samples/bpf/,
   $ make -j20 && make headers_install && make samples/bpf/
you will see a lot of failures like below:

  clang  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/7/include \
         -I/home/yhs/work/bpf-next/arch/x86/include \
         -I./arch/x86/include/generated  -I/home/yhs/work/bpf-next/include \
         -I./include -I/home/yhs/work/bpf-next/arch/x86/include/uapi \
         -I./arch/x86/include/generated/uapi \
         -I/home/yhs/work/bpf-next/include/uapi -I./include/generated/uapi \
         -include /home/yhs/work/bpf-next/include/linux/kconfig.h
         -Isamples/bpf \
	 -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf/ \
	 -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
	 -D__TARGET_ARCH_x86 -Wno-compare-distinct-pointer-types \
	 -Wno-gnu-variable-sized-type-not-at-end \
	 -Wno-address-of-packed-member -Wno-tautological-compare \
	 -Wno-unknown-warning-option  \
	 -O2 -emit-llvm -c /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c \
         -o -| llc -march=bpf -filetype=obj -o samples/bpf/xdp_redirect_cpu_kern.o
  In file included from /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c:10:
  In file included from /home/yhs/work/bpf-next/include/uapi/linux/in.h:24:
  In file included from /home/yhs/work/bpf-next/include/linux/socket.h:8:
  In file included from /home/yhs/work/bpf-next/include/linux/uio.h:13:
  In file included from /home/yhs/work/bpf-next/include/linux/thread_info.h:38:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/thread_info.h:53:
  /home/yhs/work/bpf-next/arch/x86/include/asm/cpufeature.h:150:2: error: 'asm goto' constructs are not supported yet
        asm_volatile_goto("1: jmp 6f\n"
        ^
  /home/yhs/work/bpf-next/include/linux/compiler-gcc.h:296:42: note: expanded from macro 'asm_volatile_goto'
  #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
                                           ^
  1 error generated.
  ...
  In file included from /home/yhs/work/bpf-next/samples/bpf/tracex4_kern.c:7:
  In file included from /home/yhs/work/bpf-next/include/linux/ptrace.h:6:
  In file included from /home/yhs/work/bpf-next/include/linux/sched.h:14:
  In file included from /home/yhs/work/bpf-next/include/linux/pid.h:5:
  In file included from /home/yhs/work/bpf-next/include/linux/rculist.h:11:
  In file included from /home/yhs/work/bpf-next/include/linux/rcupdate.h:40:
  In file included from /home/yhs/work/bpf-next/include/linux/preempt.h:81:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/preempt.h:7:
  In file included from /home/yhs/work/bpf-next/include/linux/thread_info.h:38:
  In file included from /home/yhs/work/bpf-next/arch/x86/include/asm/thread_info.h:53:
  /home/yhs/work/bpf-next/arch/x86/include/asm/cpufeature.h:150:2: error: 'asm goto' constructs are not supported yet
  ...

In the above, bpf compilation looks like below
  clang -target <native_arch> ... -emit-llvm -c -o - | llc -march=bpf ...
The clang compilation targets the native architecture and generates the LLVM IR bytecode, and
the llc compilation takes the IR bytecode and generates bpf instructions.

If kernel header files accessed by bpf program have asm goto construct, e.g.,
arch/x86/include/asm/cpufeature.h, the "clang -target <native_arch> ..."
compilation will fail as clang does not support asm goto yet.

Solution
=========

The solution proposed in this patch does not need user space change.
When the kernel detects the compiler has asm goto support,
it sets CC_HAVE_ASM_GOTO, the patch added the additional marco definition
__NO_CLANG_BPF_HACK. In arch/x86/include/asm/cpufeature.h, the
asm goto construct is accessed only if __NO_CLANG_BPF_HACK is defined.
This should not impact kernel compilation functionality since
for x86, we have
  ifndef CC_HAVE_ASM_GOTO
    $(error Compiler lacks asm-goto support.)
  endif
So __NO_CLANG_BPF_HACK should be always defined during kernel compilation
targeting x86.

User space code which compiles bpf program does not have
__NO_CLANG_BPF_HACK defined so it will go to alternate path
which avoids asm goto.

This approach is preferred since the already deployed bcc scripts, or
any other bpf applicaitons utilizing LLVM JIT compilation functionality,
will continue work with the new kernel without re-compilation and
re-deployment.

Note that this is a hack in the kernel to workaround bpf compilation issue.
The hack will be removed once clang starts to support asm goto.

Fixes: d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 Makefile                          | 1 +
 arch/x86/include/asm/cpufeature.h | 5 +++++
 2 files changed, 6 insertions(+)

Changelog:
 v2 -> v3:
   . Changed macro name from NO_BPF_WORKAROUND to __NO_CLANG_BPF_HACK.
     Explained the solution in more details.
 v1 -> v2:
   . Use NO_BPF_WORKAROUND macro instead of CC_HAVE_ASM_GOTO
     to make it explicit that this is a workaround.

Peter,

Could you take a look at this patch revision and give your opinion?
More and more people hit this issue and have to manually work around it.
It would be good if this can be resolved soon.

Thanks!

diff --git a/Makefile b/Makefile
index 83b6c54..cfd8759 100644
--- a/Makefile
+++ b/Makefile
@@ -504,6 +504,7 @@ export RETPOLINE_CFLAGS
 ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
   CC_HAVE_ASM_GOTO := 1
   KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
+  KBUILD_CFLAGS += -D__NO_CLANG_BPF_HACK
   KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
 endif
 
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index b27da96..42edd5d 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -140,6 +140,8 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
 
 #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
 
+/* this macro is a temporary hack for bpf until clang gains asm-goto support */
+#ifdef __NO_CLANG_BPF_HACK
 /*
  * Static testing of CPU features.  Used the same as boot_cpu_has().
  * These will statically patch the target code for additional
@@ -195,6 +197,9 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
 		boot_cpu_has(bit) :				\
 		_static_cpu_has(bit)				\
 )
+#else
+#define static_cpu_has(bit)		boot_cpu_has(bit)
+#endif
 
 #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
 #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
-- 
2.9.5

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Peter Zijlstra]

On Thu, May 03, 2018 at 08:31:19PM -0700, Yonghong Song wrote:

> This approach is preferred since the already deployed bcc scripts, or
> any other bpf applicaitons utilizing LLVM JIT compilation functionality,
> will continue work with the new kernel without re-compilation and
> re-deployment.

So I really hate this and would much rather see the BPF build
environment changed. It not consistenyly having __BPF__ defined really
smells like a bug on your end.

Sometimes you just need to update tools... Is it really too hard to do
-D__BPF__ in the bpf build process that we need to mollest the kernel
for it?

> Note that this is a hack in the kernel to workaround bpf compilation issue.
> The hack will be removed once clang starts to support asm goto.

Note that that ^^ already mandates people re-deploy their bpf tools, so
why is llvm supporting asm-goto a better point to re-deploy than fixing
a consistent __BPF__ define for the bpf build environment?

> diff --git a/Makefile b/Makefile
> index 83b6c54..cfd8759 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -504,6 +504,7 @@ export RETPOLINE_CFLAGS
>  ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
>    CC_HAVE_ASM_GOTO := 1
>    KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> +  KBUILD_CFLAGS += -D__NO_CLANG_BPF_HACK
>    KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
>  endif

I really think this is the wrong thing to do; but if the x86 maintainers
are willing to take this, I'll grudingly shut up.

Ingo, Thomas?

> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index b27da96..42edd5d 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -140,6 +140,8 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
>  
>  #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
>  
> +/* this macro is a temporary hack for bpf until clang gains asm-goto support */
> +#ifdef __NO_CLANG_BPF_HACK
>  /*
>   * Static testing of CPU features.  Used the same as boot_cpu_has().
>   * These will statically patch the target code for additional
> @@ -195,6 +197,9 @@ static __always_inline __pure bool _static_cpu_has(u16 bit)
>  		boot_cpu_has(bit) :				\
>  		_static_cpu_has(bit)				\
>  )
> +#else
> +#define static_cpu_has(bit)		boot_cpu_has(bit)
> +#endif
>  
>  #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
>  #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
> -- 
> 2.9.5
> 

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Alexei Starovoitov]

On Thu, May 10, 2018 at 12:06:34PM +0200, Peter Zijlstra wrote:
> On Thu, May 03, 2018 at 08:31:19PM -0700, Yonghong Song wrote:
> 
> > This approach is preferred since the already deployed bcc scripts, or
> > any other bpf applicaitons utilizing LLVM JIT compilation functionality,
> > will continue work with the new kernel without re-compilation and
> > re-deployment.
> 
> So I really hate this and would much rather see the BPF build
> environment changed. It not consistenyly having __BPF__ defined really
> smells like a bug on your end.
> 
> Sometimes you just need to update tools... Is it really too hard to do
> -D__BPF__ in the bpf build process that we need to mollest the kernel
> for it?
> 
> > Note that this is a hack in the kernel to workaround bpf compilation issue.
> > The hack will be removed once clang starts to support asm goto.
> 
> Note that that ^^ already mandates people re-deploy their bpf tools, so
> why is llvm supporting asm-goto a better point to re-deploy than fixing
> a consistent __BPF__ define for the bpf build environment?

This thread has been going for over a month now. Looks like we're starting
to go in circles and what we discussed earlier got lost. To recap:

- this is NOT a bpf issue. libbcc is the first user that got broken
  by commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")

- all other libraries and tools (like fuzzers, static code analyzers, etc)
  that are using clang front-end to process kernel headers are broken too

That makes me wonder what happened with "we do not break user space" rule?

I think the safest course of action would be to partially revert the
offending commit d0266046ad54 which we proposed first.
Since you were concerned that this is somehow will disincentivize clang
folks to add asm-goto support, we agreed to add this __NO_CLANG_BPF_HACK
macro to un-break libbcc at least, so that existing libbcc installations
will continue to work when people upgrade kernels.

The __BPF__ macro is automatically defined by clang when '-target bpf' is used.
This is NOT the case here. libbcc has to use native target when
processing kernel headers.

It happens from time to time that one or the other libbcc script gets
broken by new kernel because kprobe symbol that the script is using
got changed in the recent kernel. That's ok and we deal with this situation
all the time. What's different this time that ALL bcc scripts are broken
because of this commit and we cannot workaround in user space.

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Borislav Petkov]

On Thu, May 10, 2018 at 08:52:42AM -0700, Alexei Starovoitov wrote:
> That makes me wonder what happened with "we do not break user space" rule?

As someone already pointed out on IRC, arch/x86/include/asm/cpufeature.h
is solely a kernel header so nothing but kernel should include it. So
forget the userspace breakage "argument".

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Gianluca Borello]

On Thu, May 10, 2018 at 9:28 AM Borislav Petkov <bp@alien8.de> wrote:

> As someone already pointed out on IRC, arch/x86/include/asm/cpufeature.h
> is solely a kernel header so nothing but kernel should include it. So
> forget the userspace breakage "argument".


For what is worth, I have the same exact problem in a relatively popular
open source system call tracer, and my attempt to fix the issue from user
space has been:
https://github.com/draios/sysdig/commit/2958eb1d52e047f4b93d1238be803e7c405bdec2

While I can definitely live with that (and I'd be happy to submit a patch
to samples/ following the same approach) and absolutely respect the
technical authority of the reviewers here, it would be nice to recognize
that these changes actually affect users to a certain degree, even if from
a technical point of view they don't break userspace.

 From a practical point of view, BPF is widely used from userspace programs
to access some kernel data structures to gather visibility information, and
even the simplest use case, such as including linux/sched.h to access some
task_struct members, ends up pulling in arch/x86/include/asm/cpufeature.h,
thus (ab)using that file. Adding these quirks definitely increases the
complexity a developer needs to keep in mind in order to take advantage of
a BPF based instrumentation.

Thanks

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Alexei Starovoitov]

On Thu, May 10, 2018 at 06:20:28PM +0200, Borislav Petkov wrote:
> On Thu, May 10, 2018 at 08:52:42AM -0700, Alexei Starovoitov wrote:
> > That makes me wonder what happened with "we do not break user space" rule?
> 
> As someone already pointed out on IRC, arch/x86/include/asm/cpufeature.h
> is solely a kernel header so nothing but kernel should include it. So
> forget the userspace breakage "argument".

libbcc is a library. It's not only used by bcc scripts, but by production
services that compile bpf programs with clang.
Without this kernel fix the companies won't be able to upgrade the kernel.
Even if we could somehow hack libbcc and workaround in user space
(which is not possible as already explained), it's a long dependency chain
to recompile and upgrade core services before upgrading the kernel
which makes even theoretical user space fix impractical.
I see no option, but to fix the kernel.
Regardless whether it's called user space breakage or kernel breakage.

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Borislav Petkov]

On Thu, May 10, 2018 at 10:58:35AM -0700, Alexei Starovoitov wrote:
> libbcc is a library. It's not only used by bcc scripts, but by production
> services that compile bpf programs with clang.

Let me get this straight: libbcc fails to compile because it
includes (through some long include chain) the kernel header
arch/x86/include/asm/cpufeature.h.

Is that it?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto

[Borislav Petkov]

On Thu, May 03, 2018 at 08:31:19PM -0700, Yonghong Song wrote:
> Commit d0266046ad54 ("x86: Remove FAST_FEATURE_TESTS")
> removed X86_FAST_FEATURE_TESTS and make macro static_cpu_has() always
> use __always_inline function _static_cpu_has() funciton.
> The static_cpu_has() uses gcc feature asm goto construct,
> which is not supported by clang.
> 
> Issues
> ======
> 
> Currently, for BPF programs written in C, the only widely-supported
> compiler is clang. Because of clang lacking support
> of asm goto construct, if you try to compile
> bpf programs under samples/bpf/,
>    $ make -j20 && make headers_install && make samples/bpf/
> you will see a lot of failures like below:
> 
>   clang  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/7/include \
>          -I/home/yhs/work/bpf-next/arch/x86/include \
>          -I./arch/x86/include/generated  -I/home/yhs/work/bpf-next/include \
>          -I./include -I/home/yhs/work/bpf-next/arch/x86/include/uapi \
>          -I./arch/x86/include/generated/uapi \
>          -I/home/yhs/work/bpf-next/include/uapi -I./include/generated/uapi \
>          -include /home/yhs/work/bpf-next/include/linux/kconfig.h
>          -Isamples/bpf \
> 	 -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf/ \
> 	 -D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
> 	 -D__TARGET_ARCH_x86 -Wno-compare-distinct-pointer-types \
> 	 -Wno-gnu-variable-sized-type-not-at-end \
> 	 -Wno-address-of-packed-member -Wno-tautological-compare \
> 	 -Wno-unknown-warning-option  \
> 	 -O2 -emit-llvm -c /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c \
>          -o -| llc -march=bpf -filetype=obj -o samples/bpf/xdp_redirect_cpu_kern.o
>   In file included from /home/yhs/work/bpf-next/samples/bpf/xdp_redirect_cpu_kern.c:10:

This is not the kernel's problem.

Fix the samples to build successfully like the rest of the things in
tools/ use kernel machinery and undefine/redefine stuff so that it
builds. For example, tools/include/asm/alternative-asm.h redefines two
macros. So do something along those lines.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.