netdev - Re: [PATCH net-next v3 0/3] virtio_net: add optional flow dissection in virtio_net_hdr_to

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]

Date:   Fri, 4 Jun 2021 10:43:49 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Tanner Love <tannerlove.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Petar Penkov <ppenkov@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Tanner Love <tannerlove@...gle.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: [PATCH net-next v3 0/3] virtio_net: add optional flow dissection
 in virtio_net_hdr_to_skb

> >> Several questions:
> >>
> >> 1) having bpf core to know about virito-net header seems like a layer
> >> violation, it doesn't scale as we may add new fields, actually there's
> >> already fields that is not implemented in the spec but not Linux right now.
> > struct virtio_net_hdr is used by multiple interfaces, not just virtio.
> > The interface as is will remain, regardless of additional extensions.
> >
> > If the interface is extended, the validation can be extended with it.
>
>
> One possible problem is that there's no sufficient context.
>
> The vnet header length is not a fixed value but depends on the feature
> negotiation. The num_buffers (not implemented in this series) is an
> example. The field doesn't not exist for legacy device if mergeable
> buffer is disabled. If we decide to go with this way, we probably need
> to fix this by introducing a vnet header length.
>
> And I'm not sure it can work for all the future cases e.g the semantic
> of a field may vary depends on the feature negotiated, but maybe it's
> safe since it needs to set the flags.
>
> Another note is that the spec doesn't exclude the possibility to have a
> complete new vnet header format in the future. And the bpf program is
> unaware of any virtio features.

We can extend the program with a version or type field, if multiple
variants appear. The callers can set this.

Thanks for the examples. As a matter of fact, I do know that kind of
extension. I proposed new fields myself this winter, to for timestamps,
pacing offload and hash info on tx:

https://lore.kernel.org/netdev/20210208185558.995292-1-willemdebruijn.kernel@gmail.com/T/#mcbd4dff966a93d61a31844c9d968e7cd4ee7f0ab

Like num_buffers, those are new fields appended to the struct.

Agreed that if the semantics of the existing fields would change or
a whole new v2 type would be defined (with much stricter semantics
that time around, and validation from the start), then a type field in
the flow dissector will be needed.

That is feasible and won't have to break the BPF interface.

> >
> > Just curious: can you share what extra fields may be in the pipeline?
> > The struct has historically not seen (m)any changes.
>
>
> For extra fields, I vaguely remember we had some discussions on the
> possible method to extend that, but I forget the actual features.
>
> But spec support RSC which may reuse csum_start/offset but it looks to
> me RSC is not something like Linux need.
>
>
> >
> >> 2) virtio_net_hdr_to_skb() is not the single entry point, packet could
> >> go via XDP
> > Do you mean AF_XDP?
>
>
> Yes and kernel XDP as well. If the packet is redirected or transmitted,
> it won't even go to virtio_net_hdr_to_skb().

Redirected packets are already in the kernel.

This is strictly a chokepoint for new packets injected from userspace.

> Since there's no GSO/csum support for XDP, it's probably ok, but needs
> to consider this for the future consider the multi-buffer XDP is being
> developed right now, we can release those restriction.

Yes, we have to make sure not to introduce the same issues with any
XDP GSO extensions, if it comes to that.

> > As far as I know, vnet_hdr is the only injection
> > interface for complex packets that include offload instructions (GSO,
> > csum) -- which are the ones mostly implicated in bug reports.
>
>
> Ideally, if GSO/csum is supported by XDP, it would be more simple to use
> XDP I think.

That might actually reduce the odds of seeing new virtio_net_hdr extensions?

That legacy interface is here to stay, though, so we have to continue
to be prepared to handle any input that comes that way.