netdev - Re: [PATCH net-next v4 2/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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Jun 2021 11:53:40 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.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>,
        "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


在 2021/6/10 上午11:15, Willem de Bruijn 写道:
> On Wed, Jun 9, 2021 at 11:09 PM Jason Wang <jasowang@...hat.com> wrote:
>>
>> 在 2021/6/9 上午1:02, Tanner Love 写道:
>>>    retry:
>>> -                     if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
>>> +                     /* only if flow dissection not already done */
>>> +                     if (!static_branch_unlikely(&sysctl_flow_dissect_vnet_hdr_key) &&
>>> +                         !skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
>>>                                                              NULL, 0, 0, 0,
>>>                                                              0)) {
>>
>> So I still wonder the benefit we could gain from reusing the bpf flow
>> dissector here. Consider the only context we need is the flow keys,
> Maybe I misunderstand the comment, but the goal is not just to access
> the flow keys, but to correlate data in the packet payload with the fields
> of the virtio_net_header. E.g., to parse a packet payload to verify that
> it matches the gso type and header length. I don't see how we could
> make that two separate programs, one to parse a packet (flow dissector)
> and one to validate the vnet_hdr.


I may miss something here. I think it could be done via passing flow 
keys built by flow dissectors to vnet header validation eBPF program.

(Looking at validate_vnet_hdr(), it needs only flow keys and skb length).

Having two programs may have other advantages:

1) bpf vnet header validation can work without bpf flow dissector
2) keep bpf flow dissector simpler (unaware of vnet header), otherwise 
you need to prepare a dedicated bpf dissector for virtio-net/tap etc.


>
>> we
>> had two choices
>>
>> a1) embed the vnet header checking inside bpf flow dissector
>> a2) introduce a dedicated eBPF type for doing that
>>
>> And we have two ways to access the vnet header
>>
>> b1) via pesudo __sk_buff
>> b2) introduce bpf helpers
>>
>> I second for a2 and b2. The main motivation is to hide the vnet header
>> details from the bpf subsystem.
> b2 assumes that we might have many different variants of
> vnet_hdr, but that all will be able to back a functional
> interface like "return the header length"?


Probably.


> Historically, the
> struct has not seen such a rapid rate of change that I think
> would warrant introducing this level of indirection.


For the rapid change, yes. But:

1) __sk_buff is part of the general uAPI, adding virtio-net fields looks 
like a layer violation and a duplication to the existing virtio-net uAPI
2) as you said below the vnet header is not self contained
3) using helpers we can make vnet header format transparent to bpf core 
(that's the way how bpf access TCP headers AFAIK)


>
> The little_endian field does demonstrate one form of how
> the struct can be context sensitive -- and not self describing.


Yes and we probably need to provide more context, e.g the negotiated 
features.


>
> I think we can capture multiple variants of the struct, if it ever
> comes to that, with a versioning field. We did not add that
> right away, because that can be introduced later, when a
> new version arrives. But we have a plan for the eventuality.


It works but it couples virtio with bpf.

Thanks


>> Thanks
>>