Re: [PATCH bpf-next v10 0/7] Generalizing bpf_local_storage

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: KP Singh <kpsingh@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Paul Turner <pjt@google.com>,
	Jann Horn <jannh@google.com>,
	Florent Revest <revest@chromium.org>
Subject: Re: [PATCH bpf-next v10 0/7] Generalizing bpf_local_storage
Date: Tue, 25 Aug 2020 15:13:02 -0700	[thread overview]
Message-ID: <CAADnVQK0sKWa-XMUR9y28KEqMCOQhnRcAu=MDv4rU8iPwLBW1w@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQJG+vMTyuNGjWTYnWX11ZqJU-EE30UC5KPJtpv1MC78cw@mail.gmail.com>

On Tue, Aug 25, 2020 at 2:05 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Aug 25, 2020 at 11:29 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > # v9 -> v10
> >
> > - Added NULL check for inode_storage_ptr before calling
> >   bpf_local_storage_update
> > - Removed an extraneous include
> > - Rebased and added Acks / Signoff.
>
> Hmm. Though it looks good I cannot apply it, because
> test_progs -t map_ptr
> is broken:
> 2225: (18) r2 = 0xffffc900004e5004
> 2227: (b4) w1 = 58
> 2228: (63) *(u32 *)(r2 +0) = r1
>  R0=map_value(id=0,off=0,ks=4,vs=4,imm=0) R1_w=inv58
> R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3=inv49 R4=inv63
> R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0
> R7=invP8 R8=map_ptr(id=0,off=0,ks=4,vs=4,imm=0) R10=?
> ; VERIFY_TYPE(BPF_MAP_TYPE_SK_STORAGE, check_sk_storage);
> 2229: (18) r1 = 0xffffc900004e5000
> 2231: (b4) w3 = 24
> 2232: (63) *(u32 *)(r1 +0) = r3
>  R0=map_value(id=0,off=0,ks=4,vs=4,imm=0)
> R1_w=map_value(id=0,off=0,ks=4,vs=8,imm=0)
> R2_w=map_value(id=0,off=4,ks=4,vs=8,imm=0) R3_w=inv24 R4=inv63
> R5=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=inv0
> R7=invP8 R8=map_pt?
> 2233: (18) r3 = 0xffff8881f03f7000
> ; VERIFY(indirect->map_type == direct->map_type);
> 2235: (85) call unknown#195896080
> invalid func unknown#195896080
> processed 4678 insns (limit 1000000) max_states_per_insn 9
> total_states 240 peak_states 178 mark_read 11
>
> libbpf: -- END LOG --
> libbpf: failed to load program 'cgroup_skb/egress'
> libbpf: failed to load object 'map_ptr_kern'
> libbpf: failed to load BPF skeleton 'map_ptr_kern': -4007
> test_map_ptr:FAIL:skel_open_load open_load failed
> #43 map_ptr:FAIL
>
> Above 'invalid func unknown#195896080' happens
> when libbpf fails to do a relocation at runtime.
> Please debug.
> It's certainly caused by this set, but not sure why.

So I've ended up bisecting and debugging it.
It turned out that the patch 1 was responsible.
I've added the following hunk to fix it:

diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index 473665cac67e..982a2d8aa844 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -589,7 +589,7 @@ static inline int check_stack(void)
        return 1;
 }

-struct bpf_sk_storage_map {
+struct bpf_local_storage_map {
        struct bpf_map map;
 } __attribute__((preserve_access_index));

@@ -602,8 +602,8 @@ struct {

 static inline int check_sk_storage(void)
 {
-       struct bpf_sk_storage_map *sk_storage =
-               (struct bpf_sk_storage_map *)&m_sk_storage;
+       struct bpf_local_storage_map *sk_storage =
+               (struct bpf_local_storage_map *)&m_sk_storage;
        struct bpf_map *map = (struct bpf_map *)&m_sk_storage;

and pushed the whole set.
In the future please always run test_progs and test_progs-no_alu32
for every patch and submit patches only if _all_ tests are passing.
Do not assume that your change is not responsible for breakage.
