linux-kernel - Re: bpf: handling non BPF register names in inline assembly with -target bpf

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180411183902.GC12166@redhat.com>
Date:   Wed, 11 Apr 2018 15:39:02 -0300
From:   Arnaldo Carvalho de Melo <acme@...hat.com>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>, acme@...nel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: bpf: handling non BPF register names in inline assembly with
 -target bpf

Em Wed, Apr 11, 2018 at 09:37:46AM -0700, Yonghong Song escreveu:
> Hi, Arnaldo,
 
> When I studied the bpf compilation issue with latest linus/net-next
> kernel (https://patchwork.kernel.org/patch/10333829/), an alternative
> approach I tried is to use __BPF__ macro.

You mean you used an alternative approach that does _not_ use the
__BPF__ macro, right? I looked at the patch and yeah, looks sane as
well, since the kernel build process already defines that
CC_HAVE_ASM_GOTO, checking if gcc has that feature, etc.

> The following patch introduced "#ifndef __BPF__" in
> arch/x86/include/asm/asm.h for some inline assembly related to x86
> "esp" register name.
 

> ==========
>  commit ca26cffa4e4aaeb09bb9e308f95c7835cb149248
> Author: Arnaldo Carvalho de Melo <acme@...hat.com>
> Date:   Mon Dec 4 13:08:47 2017 -0300
> 
>     x86/asm: Allow again using asm.h when building for the 'bpf'
> clang target
> 
>     Up to f5caf621ee35 ("x86/asm: Fix inline asm call constraints
> for Clang")
>     we were able to use x86 headers to build to the 'bpf' clang target, as
>     done by the BPF code in tools/perf/.
>  ...
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 219faae..386a690 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -136,6 +136,7 @@
>  #endif
> 
>  #ifndef __ASSEMBLY__
> +#ifndef __BPF__
>  /*
>   * This output constraint should be used for any inline asm which
> has a "call"
>   * instruction.  Otherwise the asm may be inserted before the frame
> pointer
> @@ -145,5 +146,6 @@
>  register unsigned long current_stack_pointer asm(_ASM_SP);
>  #define ASM_CALL_CONSTRAINT "+r" (current_stack_pointer)
>  #endif
> +#endif
> ...
> ==========
> 
> I just landed a clang patch (clang 7.0.0 trunk)
> https://reviews.llvm.org/rL329823
> which will permit bpf clang target to accept ANY register
> names. In this case, the inline assembly will be accepted by clang
> and will be thrown away since variable current_stack_pointer is
> not used in bpf programs.

Ok, then that ifndef __BPF__ above will not be needed anymore, but only
people with clang > that version will be able to build tools/perf/
 
> If the inline assembly is indeed for BPF program, later llc
> AsmParser will do syntax and semantics checking again.
> 
> With the above clang patch, the above "#ifndef __BPF__" can be removed.
> You can decide when is the appropriate time to use latest clang compiler
> and remove the above "#ifndef __BPF__".

So are you proposing that we have something similar to that
CC_HAVE_ASM_GOTO check in the kernel main Makefile, to define something
like CC_HAVE_ASM_REGS (or some better name), i.e. something like:

# check for 'asm(_ASM_SP)'
ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/cc-asm-regs.sh
$(CC) $(KBUILD_CFLAGS)), y)
  CC_HAVE_ASM_REGS := 1
  KBUILD_CFLAGS += -DCC_HAVE_ASM_REGS
  KBUILD_AFLAGS += -DCC_HAVE_ASM_REGS
endif

?

- Arnaldo