Re: R1 invalid mem access 'inv'

[Vincent Li <vincent.mc.li@gmail.com>]

Thanks Yonghong for looking into this.

On Tue, Jul 13, 2021 at 8:46 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/12/21 4:38 PM, Vincent Li wrote:
> > Hi Yonghong,
> >
> >
> >
> > On Fri, Jun 18, 2021 at 12:58 PM Vincent Li <vincent.mc.li@gmail.com> wrote:
> >>
> >> Hi Yonghong,
> >>
> >> I attached the full verifier log and BPF bytecode just in case it is
> >> obvious to you, if it is not, that is ok. I tried to make sense out of
> >> it and I failed due to my limited knowledge about BPF :)
> >>
> >
> > I followed your clue on investigating how fp-200=pkt changed to
> > fp-200=inv in https://github.com/cilium/cilium/issues/16517#issuecomment-873522146
> > with previous attached complete bpf verifier log and bpf bytecode, it
> > eventually comes to following
> >
> > 0000000000004948 :
> >      2345: bf a3 00 00 00 00 00 00 r3 = r10
> >      2346: 07 03 00 00 d0 ff ff ff r3 += -48
> >      2347: b7 08 00 00 06 00 00 00 r8 = 6
> > ; return ctx_store_bytes(ctx, off, mac, ETH_ALEN, 0);
> >      2348: bf 61 00 00 00 00 00 00 r1 = r6
> >      2349: b7 02 00 00 00 00 00 00 r2 = 0
> >      2350: b7 04 00 00 06 00 00 00 r4 = 6
> >      2351: b7 05 00 00 00 00 00 00 r5 = 0
> >      2352: 85 00 00 00 09 00 00 00 call 9
> >      2353: 67 00 00 00 20 00 00 00 r0 <<= 32
> >      2354: c7 00 00 00 20 00 00 00 r0 s>>= 32
> > ; if (eth_store_daddr(ctx, (__u8 *) &vtep_mac.addr, 0) < 0)
> >      2355: c5 00 54 00 00 00 00 00 if r0 s< 0 goto +84
> >
> > my new code is eth_store_daddr(ctx, (__u8 *) &vtep_mac.addr, 0) < 0;
> > that is what i copied from other part of cilium code, eth_store_daddr
> > is:
> >
> > static __always_inline int eth_store_daddr(struct __ctx_buff *ctx,
> >
> >                                             const __u8 *mac, int off)
> > {
> > #if !CTX_DIRECT_WRITE_OK
> >          return eth_store_daddr_aligned(ctx, mac, off);
> > #else
> > ......
> > }
> >
> > and eth_store_daddr_aligned is
> >
> > static __always_inline int eth_store_daddr_aligned(struct __ctx_buff *ctx,
> >
> >                                                     const __u8 *mac, int off)
> > {
> >          return ctx_store_bytes(ctx, off, mac, ETH_ALEN, 0);
> > }
> >
> > Joe  from Cilium raised an interesting question on why the compiler
> > put ctx_store_bytes() before  if (eth_store_daddr(ctx, (__u8 *)
> > &vtep_mac.addr, 0) < 0),
> > that seems to have  fp-200=pkt changed to fp-200=inv, and indeed if I
> > skip the eth_store_daddr_aligned call, the issue is resolved, do you
> > have clue on why compiler does that?
>
> This is expected. After inlining, you got
>     if (ctx_store_bytes(...) < 0) ...
>
> So you need to do
>     ctx_store_bytes(...)
> first and then do the if condition.
>

I got workaround which is not to use eth_store_daddr_aligned, but use
__builtin_memcpy() according to
cilium commit 9c857217834 (bpf: optimized memcpy/memzero with dw-wide copies)

> Looking at the issue at https://github.com/cilium/cilium/issues/16517,
> the reason seems due to xdp_store_bytes/skb_store_bytes.
> When these helpers write some data into the stack based buffer, they
> invalidate some stack contents. I don't know whether it is a false
> postive case or not, i.e., the verifier invalidates the wrong stack
> location conservatively. This needs further investigation.
>
glad to know it is not something silly that I am doing, hope it can be
figured out eventually someday :)

>
> >
> > I have more follow-up in https://github.com/cilium/cilium/issues/16517
> > if you are interested to know the full picture.
> >
> > Appreciate it very much if you have time to look at it :)
> >
> > Vincent
> >