[PATCH v2 net] bpf: do not return NET_XMIT_xxx values on bpf_redirect

[PATCH v2 net] bpf: do not return NET_XMIT_xxx values on bpf_redirect

[Yan Zhai]

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.

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

Re: [PATCH v2 net] bpf: do not return NET_XMIT_xxx values on bpf_redirect

[Alexei Starovoitov]

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?

Please add a selftest to make sure we don't regress.

Also pls mark your patch as [PATCH v3 bpf] when you respin.

> 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
>

Re: [PATCH v2 net] bpf: do not return NET_XMIT_xxx values on bpf_redirect

[Yan Zhai]

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