struct pt_regs missing in /usr/include/ tree for eBPF program compile

struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Thomas-Mich Richter]

I work on the perf tool and its bpf support for IBM s390 and came across a
strange issue compiling tools/testing/selftests/bpf/test_verifier.c on s390x.

This is the compile error:
gcc -Wall -O2 -I../../../include/uapi -I../../../lib -I../../../../include/generated
  -DHAVE_GENHDR -I../../../include    test_verifier.c
  /root/linux-devel/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -o
  /root/linux-devel/tools/testing/selftests/bpf/test_verifier
In file included from test_verifier.c:63:0:
../../../include/uapi/linux/bpf_perf_event.h:14:17: error: field ‘regs’ has
  incomplete type struct pt_regs regs;

This shows up in test case "unpriv: spill/fill of different pointers ldx"
at line 1811.
This issue is located in file /usr/include/linux/bpf_perf_event.h which is a
copy of the linux kernels include/uapi/linux/bpf_perf_event.h.

It contains:
struct bpf_perf_event_data {
        struct pt_regs regs;
        __u64 sample_period;
};

On s390 struct pt_regs is not exported to user space and does not appear
anywhere in /usr/include.
How about other architectures beside Intel?
As far as I know 
1. the struct pt_regs contains only kernel registers, no user space registers?
2. Is part of the kernel API and should not be exported at all?

When I investigated the kernel side of the bpf() system call, the test case ends
up in functions pe_prog_is_valid_access() and pe_prog_convert_ctx_access()
via syscall(bpf)
    +--> bpf_prog_load()
         +--> find_prog_type() to load eBPF type specific verifiers
         |       pe_prog_is_valid_access() and pe_prog_convert_ctx_access()
         +--> bpf_check() to verify (and modify) the eBPF
              +--> check_vfg()
                   +--> do_check()
                        +--> check_xadd()
                             +--> check_mem_access()
                                  +--> check_ctx_access()
                                       +--> env->prog->aux->ops->is_valid_access
                                            which is set to
                                            pe_prog_is_valid_access()

Now this last function expects and verifies struct pt_regs via struct member
offsets which needs a correct struct pt_regs previously setup by user space
eBPF program.

This also requires a correct struct pt_regs in /usr/include/linux/bpf_perf_event.h
(which includes /usr/include/{linux,asm,sym}/ptrace.h

How to achieve this on a platform which does not export struct pt_regs to the
user?

Thanks a lot for your help.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Fwd: struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Daniel Borkmann]

From: Thomas-Mich Richter <tmricht@linux.vnet.ibm.com>
Date: Wed, Aug 2, 2017 at 1:22 AM
[...]
> I work on the perf tool and its bpf support for IBM s390 and came across a
> strange issue compiling tools/testing/selftests/bpf/test_verifier.c on s390x.
>
> This is the compile error:
> gcc -Wall -O2 -I../../../include/uapi -I../../../lib
> -I../../../../include/generated
>    -DHAVE_GENHDR -I../../../include    test_verifier.c
>    /root/linux-devel/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -o
>    /root/linux-devel/tools/testing/selftests/bpf/test_verifier
> In file included from test_verifier.c:63:0:
> ../../../include/uapi/linux/bpf_perf_event.h:14:17: error: field ‘regs’ has
>    incomplete type struct pt_regs regs;

I actually came across the same issue today on s390
while testing for something else.

> This shows up in test case "unpriv: spill/fill of different pointers ldx"
> at line 1811.
> This issue is located in file /usr/include/linux/bpf_perf_event.h which is a
> copy of the linux kernels include/uapi/linux/bpf_perf_event.h.
>
> It contains:
> struct bpf_perf_event_data {
>          struct pt_regs regs;
>          __u64 sample_period;
> };

Yeah, correct.

> On s390 struct pt_regs is not exported to user space and does not appear
> anywhere in /usr/include.
> How about other architectures beside Intel?
> As far as I know
> 1. the struct pt_regs contains only kernel registers, no user space registers?
> 2. Is part of the kernel API and should not be exported at all?

Looking into the tree, it appears that the following archs
export a definition of struct pt_regs as uapi typically in
arch/*/include/uapi/asm/ptrace.h: x86, sparc, power, mips,
microblaze, alpha, unicore32, parisc, score, sh, mn10300,
tile, m68k, m32r, ia64, hexagon, h8300, frv, cris, c6x.
And for these I couldn't find it: s390, arc, arm64, nios2.

Anyone knows if there's any guidance on whether they must
be exported or not? It appears at least the majority of
archs is exporting them in one way or another.

Looking at 2dbb4c05d048 ("bpf/samples: Fix PT_REGS_IP on
s390x and use it") and d912557b3460 ("samples: bpf: enable
trace samples for s390x"), this was added by Michael for
the programs themselves, which were using kernel headers
for walking structs in BPF tracing programs, so a bit
unrelated to the uapi issue actually, but given the
test_verifier has couple of test cases containing pt_regs,
they should probably do the same thing and be using kernel
headers given tracing programs inspect kernel-internal
data structures typically (see BPF tracing samples).

Now, I would like to avoid going down that road to pull
in kernel internal headers into test_verifier.c, could
we instead add a bpf_ptregs.h helper in tools/testing/selftests/bpf/,
where s390 and arm64 would put a definition to fallback when
otherwise not available? Admittedly, it's a bit of a hack
if exporting them is not an option, but 'normal' tracing
progs would consult kernel headers anyway. Thoughts?

> When I investigated the kernel side of the bpf() system call, the test case ends
> up in functions pe_prog_is_valid_access() and pe_prog_convert_ctx_access()
> via syscall(bpf)
>      +--> bpf_prog_load()
>           +--> find_prog_type() to load eBPF type specific verifiers
>           |       pe_prog_is_valid_access() and pe_prog_convert_ctx_access()
>           +--> bpf_check() to verify (and modify) the eBPF
>                +--> check_vfg()
>                     +--> do_check()
>                          +--> check_xadd()
>                               +--> check_mem_access()
>                                    +--> check_ctx_access()
>                                         +--> env->prog->aux->ops->is_valid_access
>                                              which is set to
>                                              pe_prog_is_valid_access()
>
> Now this last function expects and verifies struct pt_regs via struct member
> offsets which needs a correct struct pt_regs previously setup by user space
> eBPF program.
>
> This also requires a correct struct pt_regs in
> /usr/include/linux/bpf_perf_event.h
> (which includes /usr/include/{linux,asm,sym}/ptrace.h
>
> How to achieve this on a platform which does not export struct pt_regs to the
> user?
>
> Thanks a lot for your help.
>
> --
> Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
> --
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht
> Stuttgart, HRB 243294
>

Re: Fwd: struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Thomas-Mich Richter]

On 08/04/2017 05:28 PM, Daniel Borkmann wrote:
> From: Thomas-Mich Richter <tmricht@linux.vnet.ibm.com>
> Date: Wed, Aug 2, 2017 at 1:22 AM
> [...]
>> I work on the perf tool and its bpf support for IBM s390 and came across a
>> strange issue compiling tools/testing/selftests/bpf/test_verifier.c on s390x.
>>
>> This is the compile error:
>> gcc -Wall -O2 -I../../../include/uapi -I../../../lib
>> -I../../../../include/generated
>>    -DHAVE_GENHDR -I../../../include    test_verifier.c
>>    /root/linux-devel/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -o
>>    /root/linux-devel/tools/testing/selftests/bpf/test_verifier
>> In file included from test_verifier.c:63:0:
>> ../../../include/uapi/linux/bpf_perf_event.h:14:17: error: field ‘regs’ has
>>    incomplete type struct pt_regs regs;
> 
> I actually came across the same issue today on s390
> while testing for something else.
> 
>> This shows up in test case "unpriv: spill/fill of different pointers ldx"
>> at line 1811.
>> This issue is located in file /usr/include/linux/bpf_perf_event.h which is a
>> copy of the linux kernels include/uapi/linux/bpf_perf_event.h.
>>
>> It contains:
>> struct bpf_perf_event_data {
>>          struct pt_regs regs;
>>          __u64 sample_period;
>> };
> 
> Yeah, correct.
> 
>> On s390 struct pt_regs is not exported to user space and does not appear
>> anywhere in /usr/include.
>> How about other architectures beside Intel?
>> As far as I know
>> 1. the struct pt_regs contains only kernel registers, no user space registers?
>> 2. Is part of the kernel API and should not be exported at all?
> 
> Looking into the tree, it appears that the following archs
> export a definition of struct pt_regs as uapi typically in
> arch/*/include/uapi/asm/ptrace.h: x86, sparc, power, mips,
> microblaze, alpha, unicore32, parisc, score, sh, mn10300,
> tile, m68k, m32r, ia64, hexagon, h8300, frv, cris, c6x.
> And for these I couldn't find it: s390, arc, arm64, nios2.
> 
> Anyone knows if there's any guidance on whether they must
> be exported or not? It appears at least the majority of
> archs is exporting them in one way or another.
> 
> Looking at 2dbb4c05d048 ("bpf/samples: Fix PT_REGS_IP on
> s390x and use it") and d912557b3460 ("samples: bpf: enable
> trace samples for s390x"), this was added by Michael for
> the programs themselves, which were using kernel headers
> for walking structs in BPF tracing programs, so a bit
> unrelated to the uapi issue actually, but given the
> test_verifier has couple of test cases containing pt_regs,
> they should probably do the same thing and be using kernel
> headers given tracing programs inspect kernel-internal
> data structures typically (see BPF tracing samples).

I have looked at 2dbb4c05d048 ("bpf/samples: Fix PT_REGS_IP on
> s390x and use it"). This usage scenario is a bit different.
True it requires access to individual registers via context
pointer stored in register R1. The context pointer is
of type struct bpf_perf_event_data and the first member
is of type struct pt_regs.

The big difference is the compilation of samples/bpf/sampleip_kern.c.
It is built inside the kernel root directory .../linux and includes
./arch/s390/include/asm/ptrace.h (which defines struct pt_regs).

This is different from compiling 
tools/testing/selftests/bpf/test_verifier.c. This compilation
does not happen inside the kernel root directory ..../linux
and thus needs some type of struct pt_regs definition in 
user space.

Interestingly the test_verifier.c only needs the
size of the struct pt_regs. Both failing lines of code use
offset_of:

          BPF_ALU64_IMM(BPF_ADD, BPF_REG_2,
                        -(__s32)offsetof(struct bpf_perf_event_data,
                                         sample_period) - 8)),
which in fact subtracts the sizeof (struct pt_regs) from R2.

> 
> Now, I would like to avoid going down that road to pull
> in kernel internal headers into test_verifier.c, could
> we instead add a bpf_ptregs.h helper in tools/testing/selftests/bpf/,
> where s390 and arm64 would put a definition to fallback when
> otherwise not available? Admittedly, it's a bit of a hack
> if exporting them is not an option, but 'normal' tracing
> progs would consult kernel headers anyway. Thoughts?
> 

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Fwd: struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Heiko Carstens]

On Fri, Aug 04, 2017 at 05:28:38PM +0200, Daniel Borkmann wrote:
> From: Thomas-Mich Richter <tmricht@linux.vnet.ibm.com>
> Date: Wed, Aug 2, 2017 at 1:22 AM
> [...]
> >I work on the perf tool and its bpf support for IBM s390 and came across a
> >strange issue compiling tools/testing/selftests/bpf/test_verifier.c on s390x.
> >
> >This is the compile error:
> >gcc -Wall -O2 -I../../../include/uapi -I../../../lib
> >-I../../../../include/generated
> >   -DHAVE_GENHDR -I../../../include    test_verifier.c
> >   /root/linux-devel/tools/testing/selftests/bpf/libbpf.a -lcap -lelf -o
> >   /root/linux-devel/tools/testing/selftests/bpf/test_verifier
> >In file included from test_verifier.c:63:0:
> >../../../include/uapi/linux/bpf_perf_event.h:14:17: error: field ‘regs’ has
> >   incomplete type struct pt_regs regs;
> 
> I actually came across the same issue today on s390
> while testing for something else.
> 
> >This shows up in test case "unpriv: spill/fill of different pointers ldx"
> >at line 1811.
> >This issue is located in file /usr/include/linux/bpf_perf_event.h which is a
> >copy of the linux kernels include/uapi/linux/bpf_perf_event.h.
> >
> >It contains:
> >struct bpf_perf_event_data {
> >         struct pt_regs regs;
> >         __u64 sample_period;
> >};
> 
> Yeah, correct.
> 
> >On s390 struct pt_regs is not exported to user space and does not appear
> >anywhere in /usr/include.
> >How about other architectures beside Intel?
> >As far as I know
> >1. the struct pt_regs contains only kernel registers, no user space registers?
> >2. Is part of the kernel API and should not be exported at all?
> 
> Looking into the tree, it appears that the following archs
> export a definition of struct pt_regs as uapi typically in
> arch/*/include/uapi/asm/ptrace.h: x86, sparc, power, mips,
> microblaze, alpha, unicore32, parisc, score, sh, mn10300,
> tile, m68k, m32r, ia64, hexagon, h8300, frv, cris, c6x.
> And for these I couldn't find it: s390, arc, arm64, nios2.
> 
> Anyone knows if there's any guidance on whether they must
> be exported or not? It appears at least the majority of
> archs is exporting them in one way or another.
> 
> Looking at 2dbb4c05d048 ("bpf/samples: Fix PT_REGS_IP on
> s390x and use it") and d912557b3460 ("samples: bpf: enable
> trace samples for s390x"), this was added by Michael for
> the programs themselves, which were using kernel headers
> for walking structs in BPF tracing programs, so a bit
> unrelated to the uapi issue actually, but given the
> test_verifier has couple of test cases containing pt_regs,
> they should probably do the same thing and be using kernel
> headers given tracing programs inspect kernel-internal
> data structures typically (see BPF tracing samples).
> 
> Now, I would like to avoid going down that road to pull
> in kernel internal headers into test_verifier.c, could
> we instead add a bpf_ptregs.h helper in tools/testing/selftests/bpf/,
> where s390 and arm64 would put a definition to fallback when
> otherwise not available? Admittedly, it's a bit of a hack
> if exporting them is not an option, but 'normal' tracing
> progs would consult kernel headers anyway. Thoughts?

I really don't think that struct pt_regs is part of uapi and should be
exported. We did change the layout of the pt_regs structure more than once
and would like to be able to do so in the future as well.

It looks like this is true for arm64 as well, since there only the first n
bytes of struct pt_regs are stable via their arch specific struct
user_pt_regs. However I don't know why even the first n bytes should be
considered uapi.

In addition what about compat processes? Most architectures define their
struct pt_regs with "unsigned long" members, which have different sizes for
32/64 bit, while the structure on the kernel stack contains 64 bit
members. And as far as I know the bpf test cases want to access the kernel
stack, no? Then this seems to be broken also.

We could add the hack you outlined above, but I'd like to have the same API
for all architectures. Otherwise we keep adding special cases for
architectures which don't export pt_regs via uapi (which I think is wrong).

Re: Fwd: struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Arnd Bergmann]

On Mon, Aug 14, 2017 at 2:08 PM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
>
> I really don't think that struct pt_regs is part of uapi and should be
> exported. We did change the layout of the pt_regs structure more than once
> and would like to be able to do so in the future as well.

On some architectures, pt_regs is definitely part of the uapi, as
we define sigcontext in terms of pt_regs:

arch/cris/include/uapi/asm/sigcontext.h:        struct pt_regs regs;
/* needs to be first */
arch/m32r/include/uapi/asm/sigcontext.h:        struct pt_regs *sc_pt_regs;
arch/microblaze/include/uapi/asm/sigcontext.h:  struct pt_regs regs;
arch/powerpc/include/uapi/asm/sigcontext.h:     struct pt_regs  __user *regs;
arch/tile/include/uapi/asm/sigcontext.h: * struct sigcontext has the
same shape as struct pt_regs,
arch/unicore32/include/uapi/asm/sigcontext.h:   struct pt_regs regs;

On other architectures, they just use the same layout but different names.

arm32 also uses pt_regs in struct kvm_regs, the other ones don't:

arch/arm/include/uapi/asm/kvm.h:        struct pt_regs usr_regs;
 /* R0_usr - R14_usr, PC, CPSR */

> In addition what about compat processes? Most architectures define their
> struct pt_regs with "unsigned long" members, which have different sizes for
> 32/64 bit, while the structure on the kernel stack contains 64 bit
> members. And as far as I know the bpf test cases want to access the kernel
> stack, no? Then this seems to be broken also.

Right.

      Arnd

Re: Fwd: struct pt_regs missing in /usr/include/ tree for eBPF program compile

[Alexei Starovoitov]

On Mon, Aug 14, 2017 at 02:08:07PM +0200, Heiko Carstens wrote:
> > Now, I would like to avoid going down that road to pull
> > in kernel internal headers into test_verifier.c, could
> > we instead add a bpf_ptregs.h helper in tools/testing/selftests/bpf/,
> > where s390 and arm64 would put a definition to fallback when
> > otherwise not available? Admittedly, it's a bit of a hack
> > if exporting them is not an option, but 'normal' tracing
> > progs would consult kernel headers anyway. Thoughts?
>
> I really don't think that struct pt_regs is part of uapi and should be
> exported. We did change the layout of the pt_regs structure more than once
> and would like to be able to do so in the future as well.

I think Daniel's suggestion above it the best solution and doesn't
prevent future modification to pt_regs on s390.

> We could add the hack you outlined above, but I'd like to have the same API
> for all architectures. Otherwise we keep adding special cases for
> architectures which don't export pt_regs via uapi (which I think is wrong).

I don't see any other choice but to make this hack for s390/arm64
The programs need to be able to access the registers in the format that
kernel keeps, since the programs are attached to kprobe and perf_events
and are walking in-kernel data structures.
It's already well understood that kprobe+bpf is unstable api, so having
pt_regs being unstable on s390/arm64 doesn't make it any worse.