diff mbox series

[bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest

Message ID 73f0028461c4f3fa577e24d8d797ddd76f1d17c6.1681507058.git.lorenzo@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers fail 1 blamed authors not CCed: kuba@kernel.org; 13 maintainers not CCed: mykolal@fb.com davem@davemloft.net song@kernel.org shuah@kernel.org sdf@google.com haoluo@google.com yhs@fb.com kuba@kernel.org john.fastabend@gmail.com kpsingh@kernel.org jolsa@kernel.org hawk@kernel.org linux-kselftest@vger.kernel.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: line length of 97 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-13 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-16 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-18 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-21 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-20 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for test_maps on s390x with gcc

Commit Message

Lorenzo Bianconi April 14, 2023, 9:21 p.m. UTC 
NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
depends on the device configuration. Fix XDP_REDIRECT xdp-features in
xdp_bonding selftest loading a dummy XDP program on veth2_2 device.

Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Daniel Borkmann April 14, 2023, 9:59 p.m. UTC | #1 
On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
> NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
> depends on the device configuration. Fix XDP_REDIRECT xdp-features in
> xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
> 
> Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")

Hm, does that mean we're changing^breaking existing user behavior iff after
fccca038f300 you can only make it work by loading dummy prog?

> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> index 5e3a26b15ec6..dcbe30c81291 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> @@ -168,6 +168,17 @@ static int bonding_setup(struct skeletons *skeletons, int mode, int xmit_policy,
>   
>   		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth1_2"))
>   			return -1;
> +
> +		if (!ASSERT_OK(setns_by_name("ns_dst"), "set netns to ns_dst"))
> +			return -1;
> +
> +		/* Load a dummy XDP program on veth2_2 in order to enable
> +		 * NETDEV_XDP_ACT_NDO_XMIT feature
> +		 */
> +		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth2_2"))
> +			return -1;
> +
> +		restore_root_netns();
>   	}
>   
>   	SYS("ip -netns ns_dst link set veth2_1 master bond2");
>
Lorenzo Bianconi April 14, 2023, 10:10 p.m. UTC | #2 
> On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
> > NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
> > depends on the device configuration. Fix XDP_REDIRECT xdp-features in
> > xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
> > 
> > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
> 
> Hm, does that mean we're changing^breaking existing user behavior iff after
> fccca038f300 you can only make it work by loading dummy prog?

nope, even before in order to enable ndo_xdp_xmit for veth you should load a dummy
program on the device peer or enable gro on the device peer:

https://github.com/torvalds/linux/blob/master/drivers/net/veth.c#L477

we are just reflecting this behaviour in the xdp_features flag.

Regards,
Lorenzo

> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > index 5e3a26b15ec6..dcbe30c81291 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > @@ -168,6 +168,17 @@ static int bonding_setup(struct skeletons *skeletons, int mode, int xmit_policy,
> >   		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth1_2"))
> >   			return -1;
> > +
> > +		if (!ASSERT_OK(setns_by_name("ns_dst"), "set netns to ns_dst"))
> > +			return -1;
> > +
> > +		/* Load a dummy XDP program on veth2_2 in order to enable
> > +		 * NETDEV_XDP_ACT_NDO_XMIT feature
> > +		 */
> > +		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth2_2"))
> > +			return -1;
> > +
> > +		restore_root_netns();
> >   	}
> >   	SYS("ip -netns ns_dst link set veth2_1 master bond2");
> > 
>
Daniel Borkmann April 14, 2023, 10:15 p.m. UTC | #3 
On 4/15/23 12:10 AM, Lorenzo Bianconi wrote:
>> On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
>>> NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
>>> depends on the device configuration. Fix XDP_REDIRECT xdp-features in
>>> xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
>>>
>>> Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
>>
>> Hm, does that mean we're changing^breaking existing user behavior iff after
>> fccca038f300 you can only make it work by loading dummy prog?
> 
> nope, even before in order to enable ndo_xdp_xmit for veth you should load a dummy
> program on the device peer or enable gro on the device peer:
> 
> https://github.com/torvalds/linux/blob/master/drivers/net/veth.c#L477
> 
> we are just reflecting this behaviour in the xdp_features flag.

Ok, I'm confused then why it passed before?

>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>> ---
>>>    tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
>>> index 5e3a26b15ec6..dcbe30c81291 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
>>> @@ -168,6 +168,17 @@ static int bonding_setup(struct skeletons *skeletons, int mode, int xmit_policy,
>>>    		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth1_2"))
>>>    			return -1;
>>> +
>>> +		if (!ASSERT_OK(setns_by_name("ns_dst"), "set netns to ns_dst"))
>>> +			return -1;
>>> +
>>> +		/* Load a dummy XDP program on veth2_2 in order to enable
>>> +		 * NETDEV_XDP_ACT_NDO_XMIT feature
>>> +		 */
>>> +		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth2_2"))
>>> +			return -1;
>>> +
>>> +		restore_root_netns();
>>>    	}
>>>    	SYS("ip -netns ns_dst link set veth2_1 master bond2");
Lorenzo Bianconi April 15, 2023, 11:06 a.m. UTC | #4 
> On 4/15/23 12:10 AM, Lorenzo Bianconi wrote:
> > > On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
> > > > NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
> > > > depends on the device configuration. Fix XDP_REDIRECT xdp-features in
> > > > xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
> > > > 
> > > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
> > > 
> > > Hm, does that mean we're changing^breaking existing user behavior iff after
> > > fccca038f300 you can only make it work by loading dummy prog?
> > 
> > nope, even before in order to enable ndo_xdp_xmit for veth you should load a dummy
> > program on the device peer or enable gro on the device peer:
> > 
> > https://github.com/torvalds/linux/blob/master/drivers/net/veth.c#L477
> > 
> > we are just reflecting this behaviour in the xdp_features flag.
> 
> Ok, I'm confused then why it passed before?

ack, you are right. I guess the issue is in veth driver code. In order to
enable NETDEV_XDP_ACT_NDO_XMIT for device "veth0", we need to check the peer
veth1 configuration since the check in veth_xdp_xmit() is on the peer rx queue.
Something like:

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index e1b38fbf1dd9..4b3c6647edc6 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1262,11 +1262,12 @@ static void veth_set_xdp_features(struct net_device *dev)
 
 	peer = rtnl_dereference(priv->peer);
 	if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
+		struct veth_priv *priv_peer = netdev_priv(peer);
 		xdp_features_t val = NETDEV_XDP_ACT_BASIC |
 				     NETDEV_XDP_ACT_REDIRECT |
 				     NETDEV_XDP_ACT_RX_SG;
 
-		if (priv->_xdp_prog || veth_gro_requested(dev))
+		if (priv_peer->_xdp_prog || veth_gro_requested(peer))
 			val |= NETDEV_XDP_ACT_NDO_XMIT |
 			       NETDEV_XDP_ACT_NDO_XMIT_SG;
 		xdp_set_features_flag(dev, val);
@@ -1504,19 +1505,23 @@ static int veth_set_features(struct net_device *dev,
 {
 	netdev_features_t changed = features ^ dev->features;
 	struct veth_priv *priv = netdev_priv(dev);
+	struct net_device *peer;
 	int err;
 
 	if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) || priv->_xdp_prog)
 		return 0;
 
+	peer = rtnl_dereference(priv->peer);
 	if (features & NETIF_F_GRO) {
 		err = veth_napi_enable(dev);
 		if (err)
 			return err;
 
-		xdp_features_set_redirect_target(dev, true);
+		if (peer)
+			xdp_features_set_redirect_target(peer, true);
 	} else {
-		xdp_features_clear_redirect_target(dev);
+		if (peer)
+			xdp_features_clear_redirect_target(peer);
 		veth_napi_del(dev);
 	}
 	return 0;
@@ -1598,13 +1603,13 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			peer->max_mtu = max_mtu;
 		}
 
-		xdp_features_set_redirect_target(dev, true);
+		xdp_features_set_redirect_target(peer, true);
 	}
 
 	if (old_prog) {
 		if (!prog) {
-			if (!veth_gro_requested(dev))
-				xdp_features_clear_redirect_target(dev);
+			if (peer && !veth_gro_requested(dev))
+				xdp_features_clear_redirect_target(peer);
 
 			if (dev->flags & IFF_UP)
 				veth_disable_xdp(dev);

What do you think?

Regards,
Lorenzo

> 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >    tools/testing/selftests/bpf/prog_tests/xdp_bonding.c | 11 +++++++++++
> > > >    1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > > > index 5e3a26b15ec6..dcbe30c81291 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
> > > > @@ -168,6 +168,17 @@ static int bonding_setup(struct skeletons *skeletons, int mode, int xmit_policy,
> > > >    		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth1_2"))
> > > >    			return -1;
> > > > +
> > > > +		if (!ASSERT_OK(setns_by_name("ns_dst"), "set netns to ns_dst"))
> > > > +			return -1;
> > > > +
> > > > +		/* Load a dummy XDP program on veth2_2 in order to enable
> > > > +		 * NETDEV_XDP_ACT_NDO_XMIT feature
> > > > +		 */
> > > > +		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth2_2"))
> > > > +			return -1;
> > > > +
> > > > +		restore_root_netns();
> > > >    	}
> > > >    	SYS("ip -netns ns_dst link set veth2_1 master bond2");
Alexei Starovoitov April 17, 2023, 8:20 p.m. UTC | #5 
On Sat, Apr 15, 2023 at 4:06 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On 4/15/23 12:10 AM, Lorenzo Bianconi wrote:
> > > > On 4/14/23 11:21 PM, Lorenzo Bianconi wrote:
> > > > > NETDEV_XDP_ACT_NDO_XMIT is not enabled by default for veth driver but it
> > > > > depends on the device configuration. Fix XDP_REDIRECT xdp-features in
> > > > > xdp_bonding selftest loading a dummy XDP program on veth2_2 device.
> > > > >
> > > > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag")
> > > >
> > > > Hm, does that mean we're changing^breaking existing user behavior iff after
> > > > fccca038f300 you can only make it work by loading dummy prog?
> > >
> > > nope, even before in order to enable ndo_xdp_xmit for veth you should load a dummy
> > > program on the device peer or enable gro on the device peer:
> > >
> > > https://github.com/torvalds/linux/blob/master/drivers/net/veth.c#L477
> > >
> > > we are just reflecting this behaviour in the xdp_features flag.
> >
> > Ok, I'm confused then why it passed before?
>
> ack, you are right. I guess the issue is in veth driver code. In order to
> enable NETDEV_XDP_ACT_NDO_XMIT for device "veth0", we need to check the peer
> veth1 configuration since the check in veth_xdp_xmit() is on the peer rx queue.
> Something like:
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index e1b38fbf1dd9..4b3c6647edc6 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1262,11 +1262,12 @@ static void veth_set_xdp_features(struct net_device *dev)
>
>         peer = rtnl_dereference(priv->peer);
>         if (peer && peer->real_num_tx_queues <= dev->real_num_rx_queues) {
> +               struct veth_priv *priv_peer = netdev_priv(peer);
>                 xdp_features_t val = NETDEV_XDP_ACT_BASIC |
>                                      NETDEV_XDP_ACT_REDIRECT |
>                                      NETDEV_XDP_ACT_RX_SG;
>
> -               if (priv->_xdp_prog || veth_gro_requested(dev))
> +               if (priv_peer->_xdp_prog || veth_gro_requested(peer))
>                         val |= NETDEV_XDP_ACT_NDO_XMIT |
>                                NETDEV_XDP_ACT_NDO_XMIT_SG;
>                 xdp_set_features_flag(dev, val);
> @@ -1504,19 +1505,23 @@ static int veth_set_features(struct net_device *dev,
>  {
>         netdev_features_t changed = features ^ dev->features;
>         struct veth_priv *priv = netdev_priv(dev);
> +       struct net_device *peer;
>         int err;
>
>         if (!(changed & NETIF_F_GRO) || !(dev->flags & IFF_UP) || priv->_xdp_prog)
>                 return 0;
>
> +       peer = rtnl_dereference(priv->peer);
>         if (features & NETIF_F_GRO) {
>                 err = veth_napi_enable(dev);
>                 if (err)
>                         return err;
>
> -               xdp_features_set_redirect_target(dev, true);
> +               if (peer)
> +                       xdp_features_set_redirect_target(peer, true);
>         } else {
> -               xdp_features_clear_redirect_target(dev);
> +               if (peer)
> +                       xdp_features_clear_redirect_target(peer);
>                 veth_napi_del(dev);
>         }
>         return 0;
> @@ -1598,13 +1603,13 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>                         peer->max_mtu = max_mtu;
>                 }
>
> -               xdp_features_set_redirect_target(dev, true);
> +               xdp_features_set_redirect_target(peer, true);
>         }
>
>         if (old_prog) {
>                 if (!prog) {
> -                       if (!veth_gro_requested(dev))
> -                               xdp_features_clear_redirect_target(dev);
> +                       if (peer && !veth_gro_requested(dev))
> +                               xdp_features_clear_redirect_target(peer);
>
>                         if (dev->flags & IFF_UP)
>                                 veth_disable_xdp(dev);
>
> What do you think?

Please send an official patch.
We need to fix this regression asap.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
index 5e3a26b15ec6..dcbe30c81291 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -168,6 +168,17 @@  static int bonding_setup(struct skeletons *skeletons, int mode, int xmit_policy,
 
 		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth1_2"))
 			return -1;
+
+		if (!ASSERT_OK(setns_by_name("ns_dst"), "set netns to ns_dst"))
+			return -1;
+
+		/* Load a dummy XDP program on veth2_2 in order to enable
+		 * NETDEV_XDP_ACT_NDO_XMIT feature
+		 */
+		if (xdp_attach(skeletons, skeletons->xdp_dummy->progs.xdp_dummy_prog, "veth2_2"))
+			return -1;
+
+		restore_root_netns();
 	}
 
 	SYS("ip -netns ns_dst link set veth2_1 master bond2");