RE: [PATCH net v3] virtio-net: enable NETIF_F_GRO_HW only if GRO-related offloads are supported

[Zhud <zhud@hygon.cn>]

> On Tue, Mar 17, 2026 at 07:34:33PM +0800, Di Zhu wrote:
> > Negotiating VIRTIO_NET_F_CTRL_GUEST_OFFLOADS indicates the device
> > allows control over offload support, but the offloads that can be
> > controlled may have nothing to do with GRO (e.g., if neither
> > GUEST_TSO4 nor GUEST_TSO6 is supported).
> >
> > In such a setup, reporting NETIF_F_GRO_HW as available for the device
> > is too optimistic and misleading to the user.
> >
> > Improve the situation by masking off NETIF_F_GRO_HW unless the device
> > possesses actual GRO-related offload capabilities. Out of an abundance
> > of caution, this does not change the current behaviour for hardware
> > with just v6 or just v4 GRO: current interfaces do not allow
> > distinguishing between v6/v4 GRO, so we can't expose them to userspace
> precisely.
> >
> > Fixes: dbcf24d15388 ("virtio-net: use NETIF_F_GRO_HW instead of
> > NETIF_F_LRO")
> > Signed-off-by: Di Zhu <zhud@hygon.cn>
> > ---
> > /* v3 */
> >   -Update Fixes tag to dbcf24d15388
> >   -Refine commit message using Maintainer's "too optimistic"
> >     phrasing to clarify the risk of misleading configurations.
> >
> > /* v2 */
> >   -make the modified logic clearer
> > ---
> >  drivers/net/virtio_net.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > 72d6a9c6a5a2..b233c99925e9 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -6781,8 +6781,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> >  	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> >  	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> >  		dev->features |= NETIF_F_GRO_HW;
> > -	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > -		dev->hw_features |= NETIF_F_GRO_HW;
> >
> >  	dev->vlan_features = dev->features;
> >  	dev->xdp_features = NETDEV_XDP_ACT_BASIC |
> NETDEV_XDP_ACT_REDIRECT |
> > @@ -7058,6 +7056,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> >  	}
> >  	vi->guest_offloads_capable = vi->guest_offloads;
> >
> > +	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) &&
> > +	    (vi->guest_offloads_capable & GUEST_OFFLOAD_GRO_HW_MASK))
> > +		dev->hw_features |= NETIF_F_GRO_HW;
> > +
> 
> Hmm wait a second. Isn't this late? netdev registered, even link is up already.
> 
> Documentation/networking/netdev-features.rst says:
> 
>  1. netdev->hw_features set contains features whose state may possibly
>     be changed (enabled or disabled) for a particular device by user's
>     request.  This set should be initialized in ndo_init callback and not
>     changed later.
> 
> 
> 
> I'd say let's move it up?

"Thanks for the feedback. My initial thought was that performing the modification under 
rtnl_lock after register_netdevice() would be safe from races with ethtool or link-up. However, 
I agree that staying consistent with the documentation and initializing hw_features before the device
becomes globally visible is a much cleaner approach.

> 
> Having said that, we already have a bug:
> 
>   dev->hw_features &= ~NETIF_F_RXHASH;
> is after register, too.

To be honest, I was misled by the existing code where dev->hw_features &= ~NETIF_F_RXHASH; 
is also set after registration.


> 
> This is because we take pains to recover from these commands failing.
> Except, we completely ignore that in virtnet_set_rxfh ...
> 
> 
> Cc relevant contributors to maybe take a look at fixing all that.
> 
> 
> 
> >  	rtnl_unlock();
> >
> >  	err = virtnet_cpu_notif_add(vi);
> > --
> > 2.34.1
> >
>