* [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
@ 2023-04-14 21:21 Lorenzo Bianconi
2023-04-14 21:59 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2023-04-14 21:21 UTC (permalink / raw)
To: bpf; +Cc: netdev, lorenzo.bianconi, ast, daniel, andrii, martin.lau, joamaki
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(+)
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");
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
2023-04-14 21:21 [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest Lorenzo Bianconi
@ 2023-04-14 21:59 ` Daniel Borkmann
2023-04-14 22:10 ` Lorenzo Bianconi
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2023-04-14 21:59 UTC (permalink / raw)
To: Lorenzo Bianconi, bpf
Cc: netdev, lorenzo.bianconi, ast, andrii, martin.lau, joamaki
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");
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
2023-04-14 21:59 ` Daniel Borkmann
@ 2023-04-14 22:10 ` Lorenzo Bianconi
2023-04-14 22:15 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2023-04-14 22:10 UTC (permalink / raw)
To: Daniel Borkmann
Cc: bpf, netdev, lorenzo.bianconi, ast, andrii, martin.lau, joamaki
[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]
> 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");
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
2023-04-14 22:10 ` Lorenzo Bianconi
@ 2023-04-14 22:15 ` Daniel Borkmann
2023-04-15 11:06 ` Lorenzo Bianconi
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2023-04-14 22:15 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: bpf, netdev, lorenzo.bianconi, ast, andrii, martin.lau, joamaki
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");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
2023-04-14 22:15 ` Daniel Borkmann
@ 2023-04-15 11:06 ` Lorenzo Bianconi
2023-04-17 20:20 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Lorenzo Bianconi @ 2023-04-15 11:06 UTC (permalink / raw)
To: Daniel Borkmann
Cc: bpf, netdev, lorenzo.bianconi, ast, andrii, martin.lau, joamaki
[-- Attachment #1: Type: text/plain, Size: 4533 bytes --]
> 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");
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest
2023-04-15 11:06 ` Lorenzo Bianconi
@ 2023-04-17 20:20 ` Alexei Starovoitov
0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2023-04-17 20:20 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Daniel Borkmann, bpf, Network Development, Lorenzo Bianconi,
Alexei Starovoitov, Andrii Nakryiko, Martin KaFai Lau,
Jussi Maki
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-17 20:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-14 21:21 [PATCH bpf] selftests/bpf: fix xdp_redirect xdp-features for xdp_bonding selftest Lorenzo Bianconi
2023-04-14 21:59 ` Daniel Borkmann
2023-04-14 22:10 ` Lorenzo Bianconi
2023-04-14 22:15 ` Daniel Borkmann
2023-04-15 11:06 ` Lorenzo Bianconi
2023-04-17 20:20 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).