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

在 2021/6/17 下午10:43, Willem de Bruijn 写道:

On Wed, Jun 16, 2021 at 6:22 AM Jason Wang [off-list ref] wrote:

quoted

在 2021/6/15 下午10:47, Willem de Bruijn 写道:

quoted

quoted

quoted

quoted

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.

So strictly speaking, tuntap is also the path that the packet entering 
the receive stack.

In practice, the single page constraint is a
large constraint today. But as you point out multi-buffer is a work in
progress.

Another advantage is that XDP is per device, but it looks to me flow 
dissector is per ns.

quoted

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.

I agree.

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.

Right, if we don't restrict, we can't prevent user from using this in 2).

quoted

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?

No, I meant using a general fields like flow_keys->device_hdr. And use 
bpf helpers to access the field.

I think for mitigation, we don't care too much about the performance lose.

Thanks