Date:   Thu, 3 Jan 2019 19:12:05 +0000
From:   Martin Lau <kafai@...com>
To:     Heiko Carstens <heiko.carstens@...ibm.com>
CC:     Eugene Syromyatnikov <evgsyr@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Yonghong Song <yhs@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        "Dmitry V. Levin" <ldv@...linux.org>
Subject: Re: "bpf: Improve the info.func_info and info.func_info_rec_size
 behavior" breaks strace self tests

On Thu, Jan 03, 2019 at 12:46:13PM +0100, Heiko Carstens wrote:
> Hello,
> 
> the kernel commit 7337224fc150 ("bpf: Improve the info.func_info and
> info.func_info_rec_size behavior") breaks one of strace's self tests:
> 
> FAIL: bpf-obj_get_info_by_fd-prog-v.gen
The strace's bpf-obj_get_info_by_fd-prog-v test did fail.  However,
I failed to see how 7337224fc150 broke it.  How do you trace down to
commit 7337224fc150 and can you share your test output?

The failure I can reproduce is EFAULT.  It should have been failing
as early as "nr_jited_ksyms" is added to "struct bpf_prog_info"
in linux/bpf.h.

The sys_bpf(BPF_OBJ_GET_INFO_BY_FD, &bpf_prog_get_info_attr, ...) is called
in a loop. The bpf_prog_get_info_attr.info object is in size 104 but
bpf_prog_get_info_attr.info_len is in size 168.  Hence,
if the prog is jited, the second iteraton onwards will have nr_jited_ksyms == 1
which is asking the kernel to fill the bpf_prog_get_info_attr.info.jited_ksyms
and it is NULL.

This fix the strace's test program:
diff --git i/tests/bpf-obj_get_info_by_fd.c w/tests/bpf-obj_get_info_by_fd.c
index e95afda27b3d..953083720822 100644
--- i/tests/bpf-obj_get_info_by_fd.c
+++ w/tests/bpf-obj_get_info_by_fd.c
@@ -19,6 +19,7 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <string.h>
 #include <sys/sysmacros.h>
 #include <asm/unistd.h>
 
@@ -341,6 +342,7 @@ main(void)
 	size_t old_prog_info_len = PROG_INFO_SZ;
 
 	for (unsigned int i = 0; i < 4; i++) {
+		memset(prog_info + 1, 0, PROG_INFO_SZ - sizeof(*prog_info));
 		prog_info->jited_prog_len = 0;
 		switch (i) {
 		case 1:

After the above change:
[root@...h-fb-vm1 tests]# ./bpf-obj_get_info_by_fd-prog-v
bpf(BPF_MAP_CREATE, {map_type=BPF_MAP_TYPE_ARRAY, key_size=4, value_size=8, max_entries=1, map_flags=0, inner_map_fd=0</dev/null>, map_name="test_map", map_ifindex=0}, 48) = 3<anon_inode:bpf-map>
bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_SOCKET_FILTER, insn_cnt=9, insns=[{code=BPF_ALU64|BPF_K|BPF_MOV, dst_reg=BPF_REG_1, src_reg=BPF_REG_0, off=0, imm=0}, {code=BPF_STX|BPF_W|BPF_MEM, dst_reg=BPF_REG_10, src_reg=BPF_REG_1, off=-4, imm=0}, {code=BPF_ALU64|BPF_X|BPF_MOV, dst_reg=BPF_REG_2, src_reg=BPF_REG_10, off=0, imm=0}, {code=BPF_ALU64|BPF_K|BPF_ADD, dst_reg=BPF_REG_2, src_reg=BPF_REG_0, off=0, imm=0xfffffffc}, {code=BPF_LD|BPF_DW|BPF_IMM, dst_reg=BPF_REG_1, src_reg=BPF_REG_1, off=0, imm=0x3}, {code=BPF_LD|BPF_W|BPF_IMM, dst_reg=BPF_REG_0, src_reg=BPF_REG_0, off=0, imm=0}, {code=BPF_JMP|BPF_K|BPF_CALL, dst_reg=BPF_REG_0, src_reg=BPF_REG_0, off=0, imm=0x1}, {code=BPF_ALU64|BPF_K|BPF_MOV, dst_reg=BPF_REG_0, src_reg=BPF_REG_0, off=0, imm=0}, {code=BPF_JMP|BPF_K|BPF_EXIT, dst_reg=BPF_REG_0, src_reg=BPF_REG_0, off=0, imm=0}], license="BSD", log_level=42, log_size=4096, log_buf="", kern_version=KERNEL_VERSION(57005, 192, 222), prog_flags=0, prog_name="test_prog", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS}, 72) = 4<anon_inode:bpf-prog>
bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=3<anon_inode:bpf-map>, info_len=128 => 80, info={type=BPF_MAP_TYPE_ARRAY, id=13, key_size=4, value_size=8, max_entries=1, map_flags=0, name="test_map", ifindex=0, netns_dev=makedev(0, 0), netns_ino=0}}}, 16) = 0
bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=4<anon_inode:bpf-prog>, info_len=168, info={type=BPF_PROG_TYPE_SOCKET_FILTER, id=12, tag="\xda\xbf\x02\x07\xd1\x99\x24\x86", jited_prog_len=0 => 110, jited_prog_insns=NULL, xlated_prog_len=0 => 120, xlated_prog_insns=NULL, load_time=1360454956729, created_by_uid=0, nr_map_ids=0 => 1, map_ids=NULL, name="test_prog", ifindex=0, netns_dev=makedev(0, 0), netns_ino=0}}}, 16) = 0
bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=4<anon_inode:bpf-prog>, info_len=168, info={type=BPF_PROG_TYPE_SOCKET_FILTER, id=12, tag="\xda\xbf\x02\x07\xd1\x99\x24\x86", jited_prog_len=0, jited_prog_insns=NULL, xlated_prog_len=336, xlated_prog_insns=0x7fda42a02000, load_time=1360454956729, created_by_uid=0, nr_map_ids=2, map_ids=0x7fda429f5000, name="test_prog", ifindex=0, netns_dev=makedev(0, 0), netns_ino=0}}}, 16) = -1 EFAULT (Bad address)
bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=4<anon_inode:bpf-prog>, info_len=168, info={type=BPF_PROG_TYPE_SOCKET_FILTER, id=12, tag="\xda\xbf\x02\x07\xd1\x99\x24\x86", jited_prog_len=0 => 110, jited_prog_insns=NULL, xlated_prog_len=0 => 120, xlated_prog_insns=[], load_time=1360454956729, created_by_uid=0, nr_map_ids=0 => 1, map_ids=[], name="test_prog", ifindex=0, netns_dev=makedev(0, 0), netns_ino=0}}}, 16) = 0
bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=4<anon_inode:bpf-prog>, info_len=168, info={type=BPF_PROG_TYPE_SOCKET_FILTER, id=12, tag="\xda\xbf\x02\x07\xd1\x99\x24\x86", jited_prog_len=0 => 110, jited_prog_insns=NULL, xlated_prog_len=0 => 120, xlated_prog_insns=[], load_time=1360454956729, created_by_uid=0, nr_map_ids=2 => 1, map_ids=[13], name="test_prog", ifindex=0, netns_dev=makedev(0, 0), netns_ino=0}}}, 16) = 0
+++ exited with 0 +++

> 
> Looking into the kernel commit, it seems that the user space visible
> uapi change is intentional; even though it might break existing user
> space.
> 
> To reproduce:
> 
> git clone https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_strace_strace.git&d=DwIDAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=0KV2Bnx31aCm6v7x7mYMZ26wy_rnRwk6QrpXEXP0CIE&s=c7S2IRhn_p6FF1Z2HrhC8mAMvncVDYMPpLxhvftDlV8&e=
> cd strace
> ./bootstrap
> ./configure
> make -j $(nproc)
> cd tests
> make -j $(nproc) check
> 

Powered by blists - more mailing lists