Date:   Thu, 17 Jun 2021 10:43:05 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        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>,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Tanner Love <tannerlove@...gle.com>
Subject: Re: [PATCH net-next v4 2/3] virtio_net: add optional flow dissection
 in virtio_net_hdr_to_skb

On Wed, Jun 16, 2021 at 6:22 AM Jason Wang <jasowang@...hat.com> wrote:
>
>
> 在 2021/6/15 下午10:47, Willem de Bruijn 写道:
> >>>> Isn't virtio_net_hdr a virito-net specific metadata?
> >>> I don't think it is ever since it was also adopted for tun, tap,
> >>> pf_packet and uml. Basically, all callers of virtio_net_hdr_to_skb.
> >>
> >> For tun/tap it was used to serve the backend of the virtio-net datapath
> > The purpose of this patch series is to protect the kernel against packets
> > inserted from userspace, by adding validation at the entry point.
> >
> > Agreed that BPF programs can do unspeakable things to packets, but
> > that is a different issue (with a different trust model), and out of scope.
>
>
> Ok, I think I understand your point, so basically we had two types of
> untrusted sources for virtio_net_hdr_to_skb():
>
> 1) packet injected from userspace: tun, tap, packet
> 2) packet received from a NIC: virtio-net, uml
>
> If I understand correctly, you only care about case 1). But the method
> could also be used by case 2).
>
> For 1) the proposal should work, we only need to care about csum/gso
> stuffs instead of virtio specific attributes like num_buffers.
>
> And 2) is the one that I want to make sure it works as expected since it
> requires more context to validate and we have other untrusted NICs

Yes. I did not fully appreciate the two different use cases of this
interface. For packets entering the the receive stack from a network
device, I agree that XDP is a suitable drop filter in principle. No
need for another layer. In practice, the single page constraint is a
large constraint today. But as you point out multi-buffer is a work in
progress.

> Ideally, I think XDP is probably the best. It is designed to do such
> early packet dropping per device. But it misses some important features
> which makes it can't work today.
>
> The method proposed in this patch should work for userspace injected
> packets, but it may not work very well in the case of XDP in the future.
> Using another bpf program type may only solve the issue of vnet header
> coupling with vnet header.
>
> I wonder whether or not we can do something in the middle:
>
> 1) make sure the flow dissector works even for the case of XDP (note
> that tun support XDP)

I think that wherever an XDP program exists in the datapath, that can
do the filtering, so there is no need for additional flow dissection.

If restricting this patch series to the subset of callers that inject
into the egress path *and* one of them has an XDP hook, the scope for
this feature set is narrower.

> 2) use some general fields instead of virtio-net specific fields, e.g
> using device header instead of vnet header in the flow keys strcuture

Can you give an example of what would be in the device header?

Specific for GSO, we have two sets of constants: VIRTIO_NET_HDR_GSO_..
and SKB_GSO_.. Is the suggestion to replace the current use of the
first in field flow_keys->virtio_net_hdr.gso_type with the second in
flow_keys->gso_type?

Powered by blists - more mailing lists