Messages in this thread | | | From | Yan Zhai <> | Date | Wed, 19 Jul 2023 21:48:39 -0500 | Subject | Re: [PATCH v2 net] bpf: do not return NET_XMIT_xxx values on bpf_redirect |
| |
On Tue, Jul 18, 2023 at 10:42 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 18, 2023 at 8:30 PM Yan Zhai <yan@cloudflare.com> wrote: > > > > skb_do_redirect handles returns error code from both rx and tx path. The > > tx path codes are special, e.g. NET_XMIT_CN: they are non-negative, and > > can conflict with LWTUNNEL_XMIT_xxx values. Directly returning such code > > can cause unexpected behavior. We found at least one bug that will panic > > the kernel through KASAN report when we are redirecting packets to a > > down or carrier-down device at lwt xmit hook: > > > > https://gist.github.com/zhaiyan920/8fbac245b261fe316a7ef04c9b1eba48 > > > > Above bug is hit because NET_XMIT_CN is returned by noop_qdisc of the > > down device, and it propagates from dev_queue_xmit all way to the lwt > > logic. The result is skb that has been freed by the qdisc continues to > > neighbor subsystem and triggers the bug. > > I'm struggling to parse the above paragraph. > Where bpf prog is installed? > Is this lwt bpf prog that returns BPF_REDIRECT ? > that redirects to netdev with noop_qdisc ? > What is the topology? > Sorry for the confusion. Mentioning noop_qdisc is an explanation of what happened. The actual trigger is simple: install a bpf program on lwt route at xmit hook. It bpf_redirect packets to a device FOO. If FOO is down or carrier-down, redirected packets will crash the kernel.
> Please add a selftest to make sure we don't regress. > > Also pls mark your patch as [PATCH v3 bpf] when you respin. > Ack
> > This change converts the tx code to proper errors that lwt can consume. > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > Reported-by: Jordan Griege <jgriege@cloudflare.com> > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > --- > > v2: coding style fix; sent to netdev instead of bpf for bug fixing. > > > > --- > > net/core/filter.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 06ba0e56e369..8738c7a4701d 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -2129,6 +2129,9 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) > > ret = dev_queue_xmit(skb); > > dev_xmit_recursion_dec(); > > > > + if (unlikely(ret > 0)) > > + ret = net_xmit_errno(ret); > > + > > return ret; > > } > > > > -- > > 2.30.2 > >
--
Yan
| |