[net v6 0/2] fix bpf_redirect to ifb netdev

[net v6 0/2] fix bpf_redirect to ifb netdev

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

This patchset try to fix bpf_redirect to ifb netdev.

Tonghao Zhang (2):
  net: core: set skb useful vars in __bpf_tx_skb
  selftests: bpf: add bpf_redirect to ifb

 net/core/filter.c                             |  8 +++
 tools/testing/selftests/bpf/Makefile          |  1 +
 .../bpf/progs/test_bpf_redirect_ifb.c         | 13 ++++
 .../selftests/bpf/test_bpf_redirect_ifb.sh    | 64 +++++++++++++++++++
 4 files changed, 86 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_redirect_ifb.c
 create mode 100755 tools/testing/selftests/bpf/test_bpf_redirect_ifb.sh

-- 
v6:
* remove 2/3 from v5 https://patchwork.kernel.org/project/netdevbpf/list/?series=592355&archive=both&state=*
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: Wei Wang <weiwan@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
--
2.27.0

[net v6 1/2] net: core: set skb useful vars in __bpf_tx_skb

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

We may use bpf_redirect to redirect the packets to other
netdevice (e.g. ifb) in ingress or egress path.

The target netdevice may check the *skb_iif, *redirected
and *from_ingress. For example, if skb_iif or redirected
is 0, ifb will drop the packets.

Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: Wei Wang <weiwan@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 net/core/filter.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index a7044e98765e..c1f45d2e6b0a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2107,7 +2107,15 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
 	}
 
 	skb->dev = dev;
+	/* The target netdevice (e.g. ifb) may use the:
+	 * - redirected
+	 * - from_ingress
+	 */
+#ifdef CONFIG_NET_CLS_ACT
+	skb_set_redirected(skb, skb->tc_at_ingress);
+#else
 	skb_clear_tstamp(skb);
+#endif
 
 	dev_xmit_recursion_inc();
 	ret = dev_queue_xmit(skb);
-- 
2.27.0

Re: [net v6 1/2] net: core: set skb useful vars in __bpf_tx_skb

[Alexei Starovoitov]

On Thu, Mar 24, 2022 at 6:57 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> We may use bpf_redirect to redirect the packets to other
> netdevice (e.g. ifb) in ingress or egress path.
>
> The target netdevice may check the *skb_iif, *redirected
> and *from_ingress. For example, if skb_iif or redirected
> is 0, ifb will drop the packets.
>
> Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Antoine Tenart <atenart@kernel.org>
> Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> Cc: Wei Wang <weiwan@google.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>  net/core/filter.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index a7044e98765e..c1f45d2e6b0a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2107,7 +2107,15 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>         }
>
>         skb->dev = dev;
> +       /* The target netdevice (e.g. ifb) may use the:
> +        * - redirected
> +        * - from_ingress
> +        */
> +#ifdef CONFIG_NET_CLS_ACT
> +       skb_set_redirected(skb, skb->tc_at_ingress);
> +#else
>         skb_clear_tstamp(skb);
> +#endif

I thought Daniel Nacked it a couple times already.
Please stop this spam.

Re: [net v6 1/2] net: core: set skb useful vars in __bpf_tx_skb

[Tonghao Zhang]

On Thu, Mar 24, 2022 at 11:16 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Mar 24, 2022 at 6:57 AM <xiangxia.m.yue@gmail.com> wrote:
> >
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > We may use bpf_redirect to redirect the packets to other
> > netdevice (e.g. ifb) in ingress or egress path.
> >
> > The target netdevice may check the *skb_iif, *redirected
> > and *from_ingress. For example, if skb_iif or redirected
> > is 0, ifb will drop the packets.
> >
> > Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Cc: Song Liu <songliubraving@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: KP Singh <kpsingh@kernel.org>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Antoine Tenart <atenart@kernel.org>
> > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Cc: Wei Wang <weiwan@google.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >  net/core/filter.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index a7044e98765e..c1f45d2e6b0a 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2107,7 +2107,15 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
> >         }
> >
> >         skb->dev = dev;
> > +       /* The target netdevice (e.g. ifb) may use the:
> > +        * - redirected
> > +        * - from_ingress
> > +        */
> > +#ifdef CONFIG_NET_CLS_ACT
> > +       skb_set_redirected(skb, skb->tc_at_ingress);
> > +#else
> >         skb_clear_tstamp(skb);
> > +#endif
>
> I thought Daniel Nacked it a couple times already.
> Please stop this spam.
Hi
Daniel rejected the 2/3 patch,
https://patchwork.kernel.org/project/netdevbpf/patch/20211208145459.9590-3-xiangxia.m.yue@gmail.com/
The reasons are as follows.
* 2/3 patch adds a check in fastpath.
* on egress, redirect skb to ifb is not useful.
but this patch fixes  redirect skb to ifb on ingress. I think it is
useful for us.

Hi
Daniel, can you review this patch ?

-- 
Best regards, Tonghao

Re: [net v6 1/2] net: core: set skb useful vars in __bpf_tx_skb

[Daniel Borkmann]

On 3/25/22 1:56 AM, Tonghao Zhang wrote:
> On Thu, Mar 24, 2022 at 11:16 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Thu, Mar 24, 2022 at 6:57 AM <xiangxia.m.yue@gmail.com> wrote:
>>>
>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>
>>> We may use bpf_redirect to redirect the packets to other
>>> netdevice (e.g. ifb) in ingress or egress path.
>>>
>>> The target netdevice may check the *skb_iif, *redirected
>>> and *from_ingress. For example, if skb_iif or redirected
>>> is 0, ifb will drop the packets.
>>>
>>> Fixes: a70b506efe89 ("bpf: enforce recursion limit on redirects")
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: Jakub Kicinski <kuba@kernel.org>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>> Cc: Martin KaFai Lau <kafai@fb.com>
>>> Cc: Song Liu <songliubraving@fb.com>
>>> Cc: Yonghong Song <yhs@fb.com>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: KP Singh <kpsingh@kernel.org>
>>> Cc: Eric Dumazet <edumazet@google.com>
>>> Cc: Antoine Tenart <atenart@kernel.org>
>>> Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
>>> Cc: Wei Wang <weiwan@google.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>> ---
>>>   net/core/filter.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index a7044e98765e..c1f45d2e6b0a 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2107,7 +2107,15 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
>>>          }
>>>
>>>          skb->dev = dev;
>>> +       /* The target netdevice (e.g. ifb) may use the:
>>> +        * - redirected
>>> +        * - from_ingress
>>> +        */
>>> +#ifdef CONFIG_NET_CLS_ACT
>>> +       skb_set_redirected(skb, skb->tc_at_ingress);
>>> +#else
>>>          skb_clear_tstamp(skb);
>>> +#endif
>>
>> I thought Daniel Nacked it a couple times already.
>> Please stop this spam.
> Hi
> Daniel rejected the 2/3 patch,
> https://patchwork.kernel.org/project/netdevbpf/patch/20211208145459.9590-3-xiangxia.m.yue@gmail.com/
> The reasons are as follows.
> * 2/3 patch adds a check in fastpath.
> * on egress, redirect skb to ifb is not useful.
> but this patch fixes  redirect skb to ifb on ingress. I think it is
> useful for us.
> 
> Daniel, can you review this patch ?

Still nack, above makes tc forwarding path for BPF less predictable and could break
existing setups, e.g. if someone is relying on generic XDP. I'm certain you can
resolve this without ifb hack, ifb usage is really discouraged in general.

Thanks,
Daniel

[net v6 2/2] selftests: bpf: add bpf_redirect to ifb

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

ifb netdev is used for queueing incoming traffic for shaping.
we may run bpf progs in tc cls hook(ingress or egress), to
redirect the packets to ifb.

This patch adds this test, for bpf.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: Wei Wang <weiwan@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 tools/testing/selftests/bpf/Makefile          |  1 +
 .../bpf/progs/test_bpf_redirect_ifb.c         | 13 ++++
 .../selftests/bpf/test_bpf_redirect_ifb.sh    | 64 +++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_bpf_redirect_ifb.c
 create mode 100755 tools/testing/selftests/bpf/test_bpf_redirect_ifb.sh

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 3820608faf57..7de55ec0b0bb 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -65,6 +65,7 @@ TEST_PROGS := test_kmod.sh \
 	test_xdp_vlan_mode_native.sh \
 	test_lwt_ip_encap.sh \
 	test_tcp_check_syncookie.sh \
+	test_bpf_redirect_ifb.sh \
 	test_tc_tunnel.sh \
 	test_tc_edt.sh \
 	test_xdping.sh \
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_redirect_ifb.c b/tools/testing/selftests/bpf/progs/test_bpf_redirect_ifb.c
new file mode 100644
index 000000000000..8b960cd8786b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_bpf_redirect_ifb.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 DiDi Global */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("redirect_ifb")
+int redirect(struct __sk_buff *skb)
+{
+	return bpf_redirect(skb->ifindex + 1 /* ifbX */, 0);
+}
+
+char __license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_bpf_redirect_ifb.sh b/tools/testing/selftests/bpf/test_bpf_redirect_ifb.sh
new file mode 100755
index 000000000000..c599aa0ec22e
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_bpf_redirect_ifb.sh
@@ -0,0 +1,64 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+
+# Topology:
+# ---------
+#      n1 namespace    |     n2 namespace
+#                      |
+#      -----------     |     ----------------
+#      |  veth0  | --------- |  veth1, ifb1 |
+#      -----------   peer    ----------------
+#
+
+readonly prefix="ns-$$-"
+readonly ns1="${prefix}1"
+readonly ns2="${prefix}2"
+readonly ns1_addr=192.168.1.1
+readonly ns2_addr=192.168.1.2
+
+setup() {
+	echo "Load ifb module"
+	if ! /sbin/modprobe -q -n ifb; then
+		echo "test_bpf_redirect ifb: module ifb is not found [SKIP]"
+		exit 4
+	fi
+
+	modprobe -q ifb numifbs=0
+
+	ip netns add "${ns1}"
+	ip netns add "${ns2}"
+
+	ip link add dev veth0 mtu 1500 netns "${ns1}" type veth \
+	      peer name veth1 mtu 1500 netns "${ns2}"
+	# ifb1 created after veth1
+	ip link add dev ifb1 mtu 1500 netns "${ns2}" type ifb
+
+	ip -netns "${ns1}" link set veth0 up
+	ip -netns "${ns2}" link set veth1 up
+	ip -netns "${ns2}" link set ifb1 up
+	ip -netns "${ns1}" -4 addr add "${ns1_addr}/24" dev veth0
+	ip -netns "${ns2}" -4 addr add "${ns2_addr}/24" dev veth1
+
+	ip netns exec "${ns2}" tc qdisc add dev veth1 clsact
+}
+
+cleanup() {
+	ip netns del "${ns2}" &>/dev/null
+	ip netns del "${ns1}" &>/dev/null
+	modprobe -r ifb
+}
+
+trap cleanup EXIT
+
+setup
+
+ip netns exec "${ns2}" tc filter add dev veth1 \
+	ingress bpf direct-action obj test_bpf_redirect_ifb.o sec redirect_ifb
+ip netns exec "${ns1}" ping -W 2 -c 2 -i 0.2 -q "${ns2_addr}" &>/dev/null
+if [ $? -ne 0 ]; then
+	echo "bpf redirect to ifb on ingress path [FAILED]"
+	exit 1
+fi
+
+echo OK
-- 
2.27.0