Re: [Patch bpf-next v2 3/5] bpf: compute data_end dynamically with JIT code

On Fri, Feb 12, 2021 at 2:56 AM Lorenz Bauer [off-list ref] wrote:

On Wed, 10 Feb 2021 at 02:22, Cong Wang [off-list ref] wrote:

quoted

From: Cong Wang <redacted>

Currently, we compute ->data_end with a compile-time constant
offset of skb. But as Jakub pointed out, we can actually compute
it in eBPF JIT code at run-time, so that we can competely get
rid of ->data_end. This is similar to skb_shinfo(skb) computation
in bpf_convert_shinfo_access().

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Lorenz Bauer <redacted>
Signed-off-by: Cong Wang <redacted>

...

quoted

@@ -9520,6 +9510,29 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
        return insn - insn_buf;
 }

+static struct bpf_insn *bpf_convert_data_end_access(const struct bpf_insn *si,
+                                                   struct bpf_insn *insn)

Is it worth adding a reference to this function in skb_headlen(),
since we're basically open coding that function here?

I do not mind adding a comment for this.

quoted

+{
+       /* si->dst_reg = skb->data */
+       *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data),
+                             si->dst_reg, si->src_reg,
+                             offsetof(struct sk_buff, data));
+       /* AX = skb->len */
+       *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, len),
+                             BPF_REG_AX, si->src_reg,
+                             offsetof(struct sk_buff, len));
+       /* si->dst_reg = skb->data + skb->len */
+       *insn++ = BPF_ALU64_REG(BPF_ADD, si->dst_reg, BPF_REG_AX);
+       /* AX = skb->data_len */
+       *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data_len),
+                             BPF_REG_AX, si->src_reg,
+                             offsetof(struct sk_buff, data_len));
+       /* si->dst_reg = skb->data + skb->len - skb->data_len */
+       *insn++ = BPF_ALU64_REG(BPF_SUB, si->dst_reg, BPF_REG_AX);
+
+       return insn;
+}
+
 static u32 sk_skb_convert_ctx_access(enum bpf_access_type type,
                                     const struct bpf_insn *si,
                                     struct bpf_insn *insn_buf,

@@ -9530,12 +9543,7 @@ static u32 sk_skb_convert_ctx_access(enum bpf_access_type type,

        switch (si->off) {
        case offsetof(struct __sk_buff, data_end):
-               off  = si->off;
-               off -= offsetof(struct __sk_buff, data_end);
-               off += offsetof(struct sk_buff, cb);
-               off += offsetof(struct tcp_skb_cb, bpf.data_end);
-               *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
-                                     si->src_reg, off);
+               insn = bpf_convert_data_end_access(si, insn);

This generates a new warning:

../net/core/filter.c: In function ‘sk_skb_convert_ctx_access’:
../net/core/filter.c:9542:6: warning: unused variable ‘off’ [-Wunused-variable]
 9542 |  int off;
      |      ^~~

Good catch!

Apparently neither my compiler nor kernel-test-bot's catches this.

Thanks.