> 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 <[email protected]>
> > ---
> > /* 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
> >
>
