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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/4 上午11:51, Willem de Bruijn 写道:
> On Thu, Jun 3, 2021 at 10:55 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/6/2 上午6:18, Tanner Love 写道:
>>> From: Tanner Love <tannerlove@google.com>
>>>
>>> First patch extends the flow dissector BPF program type to accept
>>> virtio-net header members.
>>>
>>> Second patch uses this feature to add optional flow dissection in
>>> virtio_net_hdr_to_skb(). This allows admins to define permitted
>>> packets more strictly, for example dropping deprecated UDP_UFO
>>> packets.
>>>
>>> Third patch extends kselftest to cover this feature.
>>
>> I wonder why virtio maintainers is not copied in this series.
> Sorry, an oversight.


No problem.


>
>> 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.


>
> 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().

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.


> 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.


>
>> 3) I wonder whether we can simply use XDP to solve this issue (metadata
>> probably but I don't have a deep thought)
>> 4) If I understand the code correctly, it should deal with all dodgy
>> packets instead of just for virtio
> Yes. Some callers of virtio_net_hdr_to_skb, such as tun_get_user and
> virtio receive_buf, pass all packets to it. Others, like tap_get_user
> and packet_snd, only call it if a virtio_net_hdr is passed. Once we
> have a validation hook, ideally all packets need to pass it. Modifying
> callers like tap_get_user can be a simple follow-on.


Ok.

Thanks

>