lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Apr 2018 09:34:05 -0700 From: Yonghong Song <yhs@...com> To: Peter Zijlstra <peterz@...radead.org> CC: <mingo@...nel.org>, <ast@...com>, <daniel@...earbox.net>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <kernel-team@...com> Subject: Re: [PATCH v2] x86/cpufeature: guard asm_volatile_goto usage with NO_BPF_WORKAROUND Hi, Peter, Just wanted to ping again so that you did not miss the email below. Please let me know your opinion. Thanks! Yonghong On 4/23/18 9:50 AM, Yonghong Song wrote: > Hi, Peter, > > Please see comments below. > > On 4/23/18 3:52 AM, Peter Zijlstra wrote: >> On Fri, Apr 20, 2018 at 11:06:03AM -0700, Yonghong Song wrote: >>> On 4/20/18 1:19 AM, Peter Zijlstra wrote: >> >>>> Hurm, so adding __BPF__ for BPF compiles isn't an option? It seems >>>> to me >>>> having a CPP flag to identify BPF compile context might be useful in >>>> general. >>> >>> With "clang -target bpf", we already have __BPF__ defined. >>> For tracing, esp. ptrace.h is included, "clang -target <native_arch>" >>> where >>> "-target <native_arch>" can be omitted, is typically used. >> >>> The reason is the native architecture header files typically >>> include a lot of various asm related stuff where "-target bpf" cannot >>> really handle. We relay on native clang to flush out all these >>> asm constructs and only bpf program needed stuff survives >>> reach to backend compiler. >> >> So because 'clang -target bpf' is 'broken', you do a work-around using > > 'clang -target bpf' is 'broken' in this case because the x86 arch has > a lot of inline asm's in the header file where bpf target cannot handle. > For most networking related bpf programs where `asm` is rarely involved, > `clang -target bpf` works fine most of time. > >> 'clang -target <native_arch>'. But because that doesn't set __BPF__ you > > `clang -target <native_arch>` should work, regardless of whether __BPF__ > macro is setup or not. The reason it doesn't work now is due to its > lacking asm-goto support. So to use `clang -target <native_arch>` is not > really a workaround for `target bpf`. It by itself should work. > >> want to add NO_BPF_WORKAROUND to the kernel instead of adding __BPF__ to >> your build rules to better mimick -target bpf, which you should be >> using. >> >> How is that sane? Why not use 'clang -target <native_arch> -D__BPF__' > > To workaround the asm-goto issue, the suggested macro __BPF__ can be > added to user space and kernel. But note that `clang -target > <native_arch>` will not define the macro __BPF__, so this requires > user space change. > > Also, to make sure people understand that this is a WORKAROUND for > asm-goto issue and is not a lasting thing we want to support. I have > the following change for cpufeature.h: > > diff --git a/arch/x86/include/asm/cpufeature.h > b/arch/x86/include/asm/cpufeature.h > index b27da9602a6d..c832118defa1 100644 > --- a/arch/x86/include/asm/cpufeature.h > +++ b/arch/x86/include/asm/cpufeature.h > @@ -140,6 +140,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, > unsigned int bit); > > #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit) > > +#ifndef __BPF_WORKAROUND__ > /* > * Static testing of CPU features. Used the same as boot_cpu_has(). > * These will statically patch the target code for additional > @@ -195,6 +196,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)) > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 4d6a6edd4bf6..b229e5090e4a 100644 > > As mentioned above, user space needs to add this new macro definition. > Specifically for kernel/samples/bpf: > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 4d6a6edd4bf6..b229e5090e4a 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -255,7 +255,7 @@ $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h > $(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_WORKAROUND__ -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 \ > > Please let me know whether this approach is okay to you or not, > whether the name __BPF_WORKAROUND__ is better than __BPF__ or not, or we > could use the earlier approach which does not require user space change. > > Thanks!
Powered by blists - more mailing lists