On Wed, Oct 19, 2022 at 6:47 PM shaozhengchao <shaozhengchao@xxxxxxxxxx> wrote: > > > > On 2022/10/18 0:36, Stanislav Fomichev wrote: > > On Sat, Oct 15, 2022 at 2:16 AM Zhengchao Shao <shaozhengchao@xxxxxxxxxx> wrote: > >> > >> As [0] see, bpf_prog_test_run_skb() should allow user space to forward > >> 14-bytes packet via BPF_PROG_RUN instead of dropping packet directly. > >> So fix it. > >> > >> 0: https://github.com/cilium/ebpf/commit/a38fb6b5a46ab3b5639ea4d421232a10013596c0 > >> > >> Fixes: fd1894224407 ("bpf: Don't redirect packets with invalid pkt_len") > >> Signed-off-by: Zhengchao Shao <shaozhengchao@xxxxxxxxxx> > >> --- > >> net/bpf/test_run.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > >> index 13d578ce2a09..aa1b49f19ca3 100644 > >> --- a/net/bpf/test_run.c > >> +++ b/net/bpf/test_run.c > >> @@ -979,9 +979,6 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb) > >> { > >> struct qdisc_skb_cb *cb = (struct qdisc_skb_cb *)skb->cb; > >> > >> - if (!skb->len) > >> - return -EINVAL; > >> - > >> if (!__skb) > >> return 0; > >> > >> @@ -1102,6 +1099,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, > >> if (IS_ERR(data)) > >> return PTR_ERR(data); > >> > >> + if (size == ETH_HLEN) > >> + is_l2 = true; > >> + > > > > Don't think this will work? That is_l2 is there to expose proper l2/l3 > > skb for specific hooks; we can't suddenly start exposing l2 headers to > > the hooks that don't expect it. > > Does it make sense to start with a small reproducer that triggers the > > issue first? We can have a couple of cases for > > len=0/ETH_HLEN-1/ETH_HLEN+1 and trigger them from the bpf program that > > redirects to different devices (to trigger dev_is_mac_header_xmit). > > > > > Hi Stanislav: > Thank you for your review. Is_l2 is the flag of a specific > hook. Therefore, do you mean that if skb->len is equal to 0, just > add the length back? Not sure I understand your question. All I'm saying is - you can't flip that flag arbitrarily. This flag depends on the attach point that you're running the prog against. Some attach points expect packets with l2, some expect packets without l2. What about starting with a small reproducer? Does it make sense to create a small selftest that adds net namespace + fq_codel + bpf_prog_test run and do redirect ingress/egress with len 0/1...tcphdr? Because I'm not sure I 100% understand whether it's only len=0 that's problematic or some other combination as well? > > > > > > > >> ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff)); > >> if (IS_ERR(ctx)) { > >> kfree(data); > >> -- > >> 2.17.1 > >>
