netdev - Re: [PATCH net] net/packet: tpacket_rcv: do not increment ring index on drop

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: <20200310172833-mutt-send-email-mst@kernel.org>
Date:   Tue, 10 Mar 2020 17:29:55 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] net/packet: tpacket_rcv: do not increment ring index
 on drop

On Tue, Mar 10, 2020 at 11:38:16AM -0400, Willem de Bruijn wrote:
> On Tue, Mar 10, 2020 at 10:44 AM Michael S. Tsirkin <mst@...hat.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 10:16:56AM -0400, Willem de Bruijn wrote:
> > > On Tue, Mar 10, 2020 at 8:59 AM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > >
> > > > On Tue, Mar 10, 2020 at 08:49:23AM -0400, Willem de Bruijn wrote:
> > > > > On Tue, Mar 10, 2020 at 2:43 AM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 09, 2020 at 11:34:35AM -0400, Willem de Bruijn wrote:
> > > > > > > From: Willem de Bruijn <willemb@...gle.com>
> > > > > > >
> > > > > > > In one error case, tpacket_rcv drops packets after incrementing the
> > > > > > > ring producer index.
> > > > > > >
> > > > > > > If this happens, it does not update tp_status to TP_STATUS_USER and
> > > > > > > thus the reader is stalled for an iteration of the ring, causing out
> > > > > > > of order arrival.
> > > > > > >
> > > > > > > The only such error path is when virtio_net_hdr_from_skb fails due
> > > > > > > to encountering an unknown GSO type.
> > > > > > >
> > > > > > > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > I wonder whether it should drop packets with unknown GSO types at all.
> > > > > > > This consistently blinds the reader to certain packets, including
> > > > > > > recent UDP and SCTP GSO types.
> > > > > >
> > > > > > Ugh it looks like you have found a bug.  Consider a legacy userspace -
> > > > > > it was actually broken by adding USD and SCTP GSO.  I suspect the right
> > > > > > thing to do here is actually to split these packets up, not drop them.
> > > > >
> > > > > In the main virtio users, virtio_net/tun/tap, the packets will always
> > > > > arrive segmented, due to these devices not advertising hardware
> > > > > segmentation for these protocols.
> > > >
> > > > Oh right. That's good then, sorry about the noise.
> > >
> > > Not at all. Thanks for taking a look!
> > >
> > > > > So the issue is limited to users of tpacket_rcv, which is relatively
> > > > > new. There too it is limited on egress to devices that do advertise
> > > > > h/w offload. And on r/x to GRO.
> > > > >
> > > > > The UDP GSO issue precedes the fraglist GRO patch, by the way, and
> > > > > goes back to my (argh!) introduction of the feature on the egress
> > > > > path.
> > > > >
> > > > > >
> > > > > > > The peer function virtio_net_hdr_to_skb already drops any packets with
> > > > > > > unknown types, so it should be fine to add an SKB_GSO_UNKNOWN type and
> > > > > > > let the peer at least be aware of failure.
> > > > > > >
> > > > > > > And possibly add SKB_GSO_UDP_L4 and SKB_GSO_SCTP types to virtio too.
> > > > > >
> > > > > > This last one is possible for sure, but for virtio_net_hdr_from_skb
> > > > > > we'll need more flags to know whether it's safe to pass
> > > > > > these types to userspace.
> > > > >
> > > > > Can you elaborate? Since virtio_net_hdr_to_skb users already returns
> > > > > -EINVAL on unknown GSO types and its callers just drop these packets,
> > > > > it looks to me that the infra is future proof wrt adding new GSO
> > > > > types.
> > > >
> > > > Oh I mean if we do want to add new types and want to pass them to
> > > > users, then virtio_net_hdr_from_skb will need to flag so it
> > > > knows whether that will or won't confuse userspace.
> > >
> > > I'm not sure how that would work. Ignoring other tun/tap/virtio for
> > > now, just looking at tpacket, a new variant of socket option for
> > > PACKET_VNET_HDR, for every new GSO type?
> >
> > Maybe a single one with a bitmap of legal types?
> >
> > > In practice the userspace I'm aware of, and any sane implementation,
> > > will be future proof to drop and account packets whose type it cannot
> > > process. So I think we can just add new types.
> >
> > Well if packets are just dropped then userspace breaks right?
> 
> It is an improvement over the current silent discard in the kernel.
> 
> If it can count these packets, userspace becomes notified that it
> should perhaps upgrade or use ethtool to stop the kernel from
> generating certain packets.
> 
> Specifically for packet sockets, it wants to receive packets as they
> appear "on the wire". It does not have to drop these today even, but
> can easily parse the headers.
> 
> For packet sockets at least, I don't think that we want transparent
> segmentation.

Well it's GSO is in the way then it's no longer "on the wire", right?
Whether we split these back to individual skbs or we don't
it's individual packets that are on the wire. GSO just allows
passing them to the application in a more efficient way.

> 
> > So we'll really need to split up packets when this happens.