Re: [PATCH net] veth: Update XDP feature set when bringing up device

From: Paolo Abeni <pabeni@redhat.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Jesper Dangaard Brouer" <hawk@kernel.org>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Lorenzo Bianconi" <lorenzo@kernel.org>,
	"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
	"Stanislav Fomichev" <sdf@google.com>,
	"Gerhard Engleder" <gerhard@engleder-embedded.com>,
	"Simon Horman" <horms@kernel.org>
Cc: Marek Majtyka <alardam@gmail.com>,
	netdev@vger.kernel.org,  bpf@vger.kernel.org
Subject: Re: [PATCH net] veth: Update XDP feature set when bringing up device
Date: Tue, 12 Sep 2023 15:10:03 +0200	[thread overview]
Message-ID: <05c3dcacfd80076bcb09bb701eab88769818c80f.camel@redhat.com> (raw)
In-Reply-To: <8734zjlfg9.fsf@toke.dk>

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