| Message ID | 20230911135826.722295-1-toke@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7a6102aa6df0d5d032b4cbc51935d1d4cda17254 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] veth: Update XDP feature set when bringing up device | expand |
Hi, On Mon, 2023-09-11 at 15:58 +0200, Toke Høiland-Jørgensen wrote: > There's an early return in veth_set_features() if the device is in a down > state, which leads to the XDP feature flags not being updated when enabling > GRO while the device is down. Which in turn leads to XDP_REDIRECT not > working, because the redirect code now checks the flags. > > Fix this by updating the feature flags after bringing the device up. > > Before this patch: > > NETDEV_XDP_ACT_BASIC: yes > NETDEV_XDP_ACT_REDIRECT: yes > NETDEV_XDP_ACT_NDO_XMIT: no > NETDEV_XDP_ACT_XSK_ZEROCOPY: no > NETDEV_XDP_ACT_HW_OFFLOAD: no > NETDEV_XDP_ACT_RX_SG: yes > NETDEV_XDP_ACT_NDO_XMIT_SG: no > > After this patch: > > NETDEV_XDP_ACT_BASIC: yes > NETDEV_XDP_ACT_REDIRECT: yes > NETDEV_XDP_ACT_NDO_XMIT: yes > NETDEV_XDP_ACT_XSK_ZEROCOPY: no > NETDEV_XDP_ACT_HW_OFFLOAD: no > NETDEV_XDP_ACT_RX_SG: yes > NETDEV_XDP_ACT_NDO_XMIT_SG: yes > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag") > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > drivers/net/veth.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 9c6f4f83f22b..0deefd1573cf 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -1446,6 +1446,8 @@ static int veth_open(struct net_device *dev) > netif_carrier_on(peer); > } > > + veth_set_xdp_features(dev); > + > return 0; > } The patch LGTM, thanks! I think it would be nice to add some specific self-tests here. Could you please consider following-up with them? Thanks, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > Hi, > > On Mon, 2023-09-11 at 15:58 +0200, Toke Høiland-Jørgensen wrote: >> There's an early return in veth_set_features() if the device is in a down >> state, which leads to the XDP feature flags not being updated when enabling >> GRO while the device is down. Which in turn leads to XDP_REDIRECT not >> working, because the redirect code now checks the flags. >> >> Fix this by updating the feature flags after bringing the device up. >> >> Before this patch: >> >> NETDEV_XDP_ACT_BASIC: yes >> NETDEV_XDP_ACT_REDIRECT: yes >> NETDEV_XDP_ACT_NDO_XMIT: no >> NETDEV_XDP_ACT_XSK_ZEROCOPY: no >> NETDEV_XDP_ACT_HW_OFFLOAD: no >> NETDEV_XDP_ACT_RX_SG: yes >> NETDEV_XDP_ACT_NDO_XMIT_SG: no >> >> After this patch: >> >> NETDEV_XDP_ACT_BASIC: yes >> NETDEV_XDP_ACT_REDIRECT: yes >> NETDEV_XDP_ACT_NDO_XMIT: yes >> NETDEV_XDP_ACT_XSK_ZEROCOPY: no >> NETDEV_XDP_ACT_HW_OFFLOAD: no >> NETDEV_XDP_ACT_RX_SG: yes >> NETDEV_XDP_ACT_NDO_XMIT_SG: yes >> >> Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag") >> Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> drivers/net/veth.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/veth.c b/drivers/net/veth.c >> index 9c6f4f83f22b..0deefd1573cf 100644 >> --- a/drivers/net/veth.c >> +++ b/drivers/net/veth.c >> @@ -1446,6 +1446,8 @@ static int veth_open(struct net_device *dev) >> netif_carrier_on(peer); >> } >> >> + veth_set_xdp_features(dev); >> + >> return 0; >> } > > The patch LGTM, thanks! > > I think it would be nice to add some specific self-tests here. Could > you please consider following-up with them? Sure! Do you want me to resubmit this as well, or are you just going to apply it as-is and do the selftest as a follow-up? -Toke
On Tue, 2023-09-12 at 14:54 +0200, Toke Høiland-Jørgensen wrote: > Paolo Abeni <pabeni@redhat.com> writes: > > > Hi, > > > > On Mon, 2023-09-11 at 15:58 +0200, Toke Høiland-Jørgensen wrote: > > > There's an early return in veth_set_features() if the device is in a down > > > state, which leads to the XDP feature flags not being updated when enabling > > > GRO while the device is down. Which in turn leads to XDP_REDIRECT not > > > working, because the redirect code now checks the flags. > > > > > > Fix this by updating the feature flags after bringing the device up. > > > > > > Before this patch: > > > > > > NETDEV_XDP_ACT_BASIC: yes > > > NETDEV_XDP_ACT_REDIRECT: yes > > > NETDEV_XDP_ACT_NDO_XMIT: no > > > NETDEV_XDP_ACT_XSK_ZEROCOPY: no > > > NETDEV_XDP_ACT_HW_OFFLOAD: no > > > NETDEV_XDP_ACT_RX_SG: yes > > > NETDEV_XDP_ACT_NDO_XMIT_SG: no > > > > > > After this patch: > > > > > > NETDEV_XDP_ACT_BASIC: yes > > > NETDEV_XDP_ACT_REDIRECT: yes > > > NETDEV_XDP_ACT_NDO_XMIT: yes > > > NETDEV_XDP_ACT_XSK_ZEROCOPY: no > > > NETDEV_XDP_ACT_HW_OFFLOAD: no > > > NETDEV_XDP_ACT_RX_SG: yes > > > NETDEV_XDP_ACT_NDO_XMIT_SG: yes > > > > > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag") > > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") > > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > > > --- > > > drivers/net/veth.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > > > index 9c6f4f83f22b..0deefd1573cf 100644 > > > --- a/drivers/net/veth.c > > > +++ b/drivers/net/veth.c > > > @@ -1446,6 +1446,8 @@ static int veth_open(struct net_device *dev) > > > netif_carrier_on(peer); > > > } > > > > > > + veth_set_xdp_features(dev); > > > + > > > return 0; > > > } > > > > The patch LGTM, thanks! > > > > I think it would be nice to add some specific self-tests here. Could > > you please consider following-up with them? > > Sure! Do you want me to resubmit this as well, or are you just going to > apply it as-is and do the selftest as a follow-up? I think the latter is simpler and works for me. The self-test could target net-next, the fix is going to land there shortly after -net. Thanks! Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On Tue, 2023-09-12 at 14:54 +0200, Toke Høiland-Jørgensen wrote: >> Paolo Abeni <pabeni@redhat.com> writes: >> >> > Hi, >> > >> > On Mon, 2023-09-11 at 15:58 +0200, Toke Høiland-Jørgensen wrote: >> > > There's an early return in veth_set_features() if the device is in a down >> > > state, which leads to the XDP feature flags not being updated when enabling >> > > GRO while the device is down. Which in turn leads to XDP_REDIRECT not >> > > working, because the redirect code now checks the flags. >> > > >> > > Fix this by updating the feature flags after bringing the device up. >> > > >> > > Before this patch: >> > > >> > > NETDEV_XDP_ACT_BASIC: yes >> > > NETDEV_XDP_ACT_REDIRECT: yes >> > > NETDEV_XDP_ACT_NDO_XMIT: no >> > > NETDEV_XDP_ACT_XSK_ZEROCOPY: no >> > > NETDEV_XDP_ACT_HW_OFFLOAD: no >> > > NETDEV_XDP_ACT_RX_SG: yes >> > > NETDEV_XDP_ACT_NDO_XMIT_SG: no >> > > >> > > After this patch: >> > > >> > > NETDEV_XDP_ACT_BASIC: yes >> > > NETDEV_XDP_ACT_REDIRECT: yes >> > > NETDEV_XDP_ACT_NDO_XMIT: yes >> > > NETDEV_XDP_ACT_XSK_ZEROCOPY: no >> > > NETDEV_XDP_ACT_HW_OFFLOAD: no >> > > NETDEV_XDP_ACT_RX_SG: yes >> > > NETDEV_XDP_ACT_NDO_XMIT_SG: yes >> > > >> > > Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag") >> > > Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") >> > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> > > --- >> > > drivers/net/veth.c | 2 ++ >> > > 1 file changed, 2 insertions(+) >> > > >> > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c >> > > index 9c6f4f83f22b..0deefd1573cf 100644 >> > > --- a/drivers/net/veth.c >> > > +++ b/drivers/net/veth.c >> > > @@ -1446,6 +1446,8 @@ static int veth_open(struct net_device *dev) >> > > netif_carrier_on(peer); >> > > } >> > > >> > > + veth_set_xdp_features(dev); >> > > + >> > > return 0; >> > > } >> > >> > The patch LGTM, thanks! >> > >> > I think it would be nice to add some specific self-tests here. Could >> > you please consider following-up with them? >> >> Sure! Do you want me to resubmit this as well, or are you just going to >> apply it as-is and do the selftest as a follow-up? > > I think the latter is simpler and works for me. The self-test could > target net-next, the fix is going to land there shortly after -net. ACK, SGTM! -Toke
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Mon, 11 Sep 2023 15:58:25 +0200 you wrote: > There's an early return in veth_set_features() if the device is in a down > state, which leads to the XDP feature flags not being updated when enabling > GRO while the device is down. Which in turn leads to XDP_REDIRECT not > working, because the redirect code now checks the flags. > > Fix this by updating the feature flags after bringing the device up. > > [...] Here is the summary with links: - [net] veth: Update XDP feature set when bringing up device https://git.kernel.org/netdev/net/c/7a6102aa6df0 You are awesome, thank you!
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 9c6f4f83f22b..0deefd1573cf 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -1446,6 +1446,8 @@ static int veth_open(struct net_device *dev) netif_carrier_on(peer); } + veth_set_xdp_features(dev); + return 0; }
There's an early return in veth_set_features() if the device is in a down state, which leads to the XDP feature flags not being updated when enabling GRO while the device is down. Which in turn leads to XDP_REDIRECT not working, because the redirect code now checks the flags. Fix this by updating the feature flags after bringing the device up. Before this patch: NETDEV_XDP_ACT_BASIC: yes NETDEV_XDP_ACT_REDIRECT: yes NETDEV_XDP_ACT_NDO_XMIT: no NETDEV_XDP_ACT_XSK_ZEROCOPY: no NETDEV_XDP_ACT_HW_OFFLOAD: no NETDEV_XDP_ACT_RX_SG: yes NETDEV_XDP_ACT_NDO_XMIT_SG: no After this patch: NETDEV_XDP_ACT_BASIC: yes NETDEV_XDP_ACT_REDIRECT: yes NETDEV_XDP_ACT_NDO_XMIT: yes NETDEV_XDP_ACT_XSK_ZEROCOPY: no NETDEV_XDP_ACT_HW_OFFLOAD: no NETDEV_XDP_ACT_RX_SG: yes NETDEV_XDP_ACT_NDO_XMIT_SG: yes Fixes: fccca038f300 ("veth: take into account device reconfiguration for xdp_features flag") Fixes: 66c0e13ad236 ("drivers: net: turn on XDP features") Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- drivers/net/veth.c | 2 ++ 1 file changed, 2 insertions(+)