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

在 2021/6/4 上午11:51, Willem de Bruijn 写道:

On Thu, Jun 3, 2021 at 10:55 PM Jason Wang [off-list ref] wrote:

quoted

在 2021/6/2 上午6:18, Tanner Love 写道:

quoted

From: Tanner Love <redacted>

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.

quoted

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.

quoted

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.

quoted

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