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

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

[Di Zhu]

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;
+
 	rtnl_unlock();
 
 	err = virtnet_cpu_notif_add(vi);
-- 
2.34.1

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

[Michael S. Tsirkin]

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?





Having said that, we already have a bug:

  dev->hw_features &= ~NETIF_F_RXHASH;                                                                                                   
is after register, too.

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
> 

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

[Zhud]

> 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
> >
> 

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

[Jakub Kicinski]

On Tue, 17 Mar 2026 19:34:33 +0800 Di Zhu wrote:
> In such a setup, reporting NETIF_F_GRO_HW as available for the device
> is too optimistic and misleading to the user.

This is not a sufficient justification to make this a fix.
As Michael was already alluding to HW GRO usually covers fewer
protocols and is less effective than SW GRO. Normal config is
to have both enabled. Please explain if there's anything special
about virtio here, because I haven't heard of people disabling
SW GRO because HW GRO is already enabled.

I think this should continue to target net-next with no Fixes tag.

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

[Zhud]

> On Tue, 17 Mar 2026 19:34:33 +0800 Di Zhu wrote:
> > In such a setup, reporting NETIF_F_GRO_HW as available for the device
> > is too optimistic and misleading to the user.
> 
> This is not a sufficient justification to make this a fix.
> As Michael was already alluding to HW GRO usually covers fewer
> protocols and is less effective than SW GRO. Normal config is
> to have both enabled. Please explain if there's anything special
> about virtio here, because I haven't heard of people disabling
> SW GRO because HW GRO is already enabled.

"Thanks, Jakub. You're right, that was a poorly chosen example to justify this as a fix, 
and I see your point regarding HW GRO vs SW GRO. There isn't anything special about
virtio here that would necessitate disabling SW GRO in normal cases; my initial thought 
was more about the potential for user confusion in specific testing scenarios.


> 
> I think this should continue to target net-next with no Fixes tag.

I'll drop the Fixes tag and target net-next in V2. I'll also move the 
hw_features initialization before register_netdevice() as discussed, 
to ensure the driver follows the proper pattern."