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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 10 Mar 2020 10:16:56 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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 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?

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.

In the worst case, arrival of these packets is under admin control with ethtool.