netdev - Re: [PATCH net-next] virtio-net: ethtool configurable LRO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-Lb8kNe1u6L18mhdz5enurWrc40SHvX0s+7-=6T=DXNyQ@mail.gmail.com>
Date:   Thu, 20 Dec 2018 13:30:15 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Network Development <netdev@...r.kernel.org>
Cc:     David Miller <davem@...emloft.net>,
        Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next] virtio-net: ethtool configurable LRO

On Wed, Dec 19, 2018 at 5:44 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Wed, Dec 19, 2018 at 5:32 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > Virtio-net devices negotiate LRO support with the host.
> > Display the initially negotiated state with ethtool -k.
> >
> > Also allow configuring it with ethtool -K, reusing the existing
> > virtnet_set_guest_offloads helper that disables LRO for XDP.
> >
> > Virtio-net negotiates TSO4 and TSO6 separately, but ethtool does not
> > distinguish between the two. In the unlikely legacy case where the
> > host only supports one of the two, maintain the current state.
> >
> > RTNL is held while calling virtnet_set_features, same as on the path
> > from virtnet_xdp_set.
> >
> > Tested:
> >   Verified that large-receive-offload is "off [fixed]" when not
> >   negotiated during initial probe and "on" otherwise.
> >
> >   Measured packet size when scp-ing a large file to the guest with
> >   nstat (IpExtInOctets / IpInDelivers). Verified that packet size
> >   exceeds MTU only if ethtool -k shows LRO and/or GRO as enabled.
> >
> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
>
> Two comments for the review:
>
> 1. Current drivers show LRO as disabled even if it is negotiated at
> probe. That is arguably a bug. In which case I can split this patch into
> - a net patch to advertise the initially negotiated value through dev->feature
> - a net-next patch to configure it with ndo_set_features.
>
> 2. virtnet_clear_guest_offloads disables all guest_offloads, including
> VIRTIO_NET_F_GUEST_CSUM. That seems undesirable? The feature is a
> prerequisite for LRO, but not the other way around. This applies
> equally to the xdp and ethtool paths, so would be another  separate
> net patch.

I was wrong. This was a deliberate choice. See below. Will leave as is.

The other change is not really important enough on its, so I'll just
send a v2 of this patch.

One additional change, show LRO as enabled if it is active for any protocol
(as opposed to only for all protocols), so that we can truly disable it in all
cases.

"
commit e59ff2c49ae16e1d179de679aca81405829aee6c
Author: Jason Wang <jasowang@...hat.com>
Date:   Thu Nov 22 14:36:30 2018 +0800

    virtio-net: disable guest csum during XDP set

    We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means we
    can receive partial csumed packets with metadata kept in the
    vnet_hdr. This may have several side effects:

    - It could be overridden by header adjustment, thus is might be not
      correct after XDP processing.
    - There's no way to pass such metadata information through
      XDP_REDIRECT to another driver.
    - XDP does not support checksum offload right now.

    So simply disable guest csum if possible in this the case of XDP.
"