On Thu, Jul 15, 2021 at 6:33 PM Vincent Li <vincent.mc.li@xxxxxxxxx> wrote: > > Thanks Yonghong for looking into this. > > On Tue, Jul 13, 2021 at 8:46 AM Yonghong Song <yhs@xxxxxx> 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@xxxxxxxxx> 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 > > > oops, I meant to this one, Finally bring an end to my long time mystery issue of using eth_store_daddr() result in "invalid mem access", I need to initialize the mac address variable with 0, uninitialized variable also cause "invalid read from stack off" in kernel 4.9, but not version above 4.9. https://github.com/cilium/cilium/pull/17370#issuecomment-922396415
