On 08/04/2017 05:28 PM, Daniel Borkmann wrote:
> From: Thomas-Mich Richter <tmri...@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
