netdev - Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable 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] [day] [month] [year] [list]

Message-ID: <20161215183341-mutt-send-email-mst@kernel.org>
Date:   Thu, 15 Dec 2016 18:35:14 +0200
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     daniel@...earbox.net, shm@...ulusnetworks.com, davem@...emloft.net,
        tgraf@...g.ch, alexei.starovoitov@...il.com,
        john.r.fastabend@...el.com, netdev@...r.kernel.org,
        brouer@...hat.com
Subject: Re: [net-next PATCH v5 1/6] net: virtio dynamically disable/enable
 LRO

On Wed, Dec 14, 2016 at 09:01:27AM -0800, John Fastabend wrote:
> On 16-12-14 05:31 AM, Michael S. Tsirkin wrote:
> > On Thu, Dec 08, 2016 at 04:04:58PM -0800, John Fastabend wrote:
> >> On 16-12-08 01:36 PM, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 07, 2016 at 12:11:11PM -0800, John Fastabend wrote:
> >>>> This adds support for dynamically setting the LRO feature flag. The
> >>>> message to control guest features in the backend uses the
> >>>> CTRL_GUEST_OFFLOADS msg type.
> >>>>
> >>>> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
> >>>> ---
> 
> [...]
> 
> >>>>  
> >>>>  static void virtnet_config_changed_work(struct work_struct *work)
> >>>> @@ -1815,6 +1846,12 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>>>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> >>>>  		dev->features |= NETIF_F_RXCSUM;
> >>>>  
> >>>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) &&
> >>>> +	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6)) {
> >>>> +		dev->features |= NETIF_F_LRO;
> >>>> +		dev->hw_features |= NETIF_F_LRO;
> >>>
> >>> So the issue is I think that the virtio "LRO" isn't really
> >>> LRO, it's typically just GRO forwarded to guests.
> >>> So these are easily re-split along MTU boundaries,
> >>> which makes it ok to forward these across bridges.
> >>>
> >>> It's not nice that we don't document this in the spec,
> >>> but it's the reality and people rely on this.
> >>>
> >>> For now, how about doing a custom thing and just disable/enable
> >>> it as XDP is attached/detached?
> >>
> >> The annoying part about doing this is ethtool will say that it is fixed
> >> yet it will be changed by seemingly unrelated operation. I'm not sure I
> >> like the idea to start automatically configuring the link via xdp_set.
> > 
> > I really don't like the idea of dropping performance
> > by a factor of 3 for people bridging two virtio net
> > interfaces.
> > 
> > So how about a simple approach for now, just disable
> > XDP if GUEST_TSO is enabled?
> > 
> > We can discuss better approaches in next version.
> > 
> 
> So the proposal is to add a check in XDP setup so that
> 
>   if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO{4|6})
> 	return -ENOPSUPP;
> 
> Or whatever is the most appropriate return code? Then we can
> disable TSO via qemu-system with guest_tso4=off,guest_tso6=off for
> XDP use cases.

Right. It's a start.

> Sounds like a reasonable start to me. I'll make the change should this
> go through DaveMs net-next tree or do you want it on virtio tree? Either
> is fine with me.
> 
> Thanks,
> John

I think I'll merge it because I'm tweaking RX processing too,
and this will likely conflict.

-- 
MST