| Message ID | 20210601221841.1251830-1-tannerlove.kernel@gmail.com |
|---|---|
| Headers | show |
| Series | virtio_net: add optional flow dissection in virtio_net_hdr_to_skb | expand |
From: Tanner Love <tannerlove.kernel@gmail.com> Date: Tue, 1 Jun 2021 18:18:37 -0400 > 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. Definitely need some bpf review of these changes. Alexei, Daniel? Thanks.
On Wed, Jun 2, 2021 at 1:10 PM David Miller <davem@davemloft.net> wrote: > > From: Tanner Love <tannerlove.kernel@gmail.com> > Date: Tue, 1 Jun 2021 18:18:37 -0400 > > > 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. > > Definitely need some bpf review of these changes. Yeah. imo it's more bpf material than net. We'll process it.
在 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. 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. 2) virtio_net_hdr_to_skb() is not the single entry point, packet could go via XDP 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 Thanks > > Tanner Love (3): > net: flow_dissector: extend bpf flow dissector support with vnet hdr > virtio_net: add optional flow dissection in virtio_net_hdr_to_skb > selftests/net: amend bpf flow dissector prog to do vnet hdr validation > > drivers/net/bonding/bond_main.c | 2 +- > include/linux/skbuff.h | 26 ++- > include/linux/virtio_net.h | 25 ++- > include/net/flow_dissector.h | 6 + > include/uapi/linux/bpf.h | 6 + > net/core/filter.c | 55 +++++ > net/core/flow_dissector.c | 27 ++- > net/core/sysctl_net_core.c | 9 + > tools/include/uapi/linux/bpf.h | 6 + > tools/testing/selftests/bpf/progs/bpf_flow.c | 188 +++++++++++++----- > .../selftests/bpf/test_flow_dissector.c | 181 +++++++++++++++-- > .../selftests/bpf/test_flow_dissector.sh | 19 ++ > 12 files changed, 470 insertions(+), 80 deletions(-) >
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. > 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. Just curious: can you share what extra fields may be in the pipeline? The struct has historically not seen (m)any changes. > 2) virtio_net_hdr_to_skb() is not the single entry point, packet could > go via XDP Do you mean AF_XDP? 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. > 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.
在 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 >
> >> 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. We can extend the program with a version or type field, if multiple variants appear. The callers can set this. Thanks for the examples. As a matter of fact, I do know that kind of extension. I proposed new fields myself this winter, to for timestamps, pacing offload and hash info on tx: https://lore.kernel.org/netdev/20210208185558.995292-1-willemdebruijn.kernel@gmail.com/T/#mcbd4dff966a93d61a31844c9d968e7cd4ee7f0ab Like num_buffers, those are new fields appended to the struct. Agreed that if the semantics of the existing fields would change or a whole new v2 type would be defined (with much stricter semantics that time around, and validation from the start), then a type field in the flow dissector will be needed. That is feasible and won't have to break the BPF interface. > > > > 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(). Redirected packets are already in the kernel. This is strictly a chokepoint for new packets injected from userspace. > 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. Yes, we have to make sure not to introduce the same issues with any XDP GSO extensions, if it comes to that. > > 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. That might actually reduce the odds of seeing new virtio_net_hdr extensions? That legacy interface is here to stay, though, so we have to continue to be prepared to handle any input that comes that way.
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. Tanner Love (3): net: flow_dissector: extend bpf flow dissector support with vnet hdr virtio_net: add optional flow dissection in virtio_net_hdr_to_skb selftests/net: amend bpf flow dissector prog to do vnet hdr validation drivers/net/bonding/bond_main.c | 2 +- include/linux/skbuff.h | 26 ++- include/linux/virtio_net.h | 25 ++- include/net/flow_dissector.h | 6 + include/uapi/linux/bpf.h | 6 + net/core/filter.c | 55 +++++ net/core/flow_dissector.c | 27 ++- net/core/sysctl_net_core.c | 9 + tools/include/uapi/linux/bpf.h | 6 + tools/testing/selftests/bpf/progs/bpf_flow.c | 188 +++++++++++++----- .../selftests/bpf/test_flow_dissector.c | 181 +++++++++++++++-- .../selftests/bpf/test_flow_dissector.sh | 19 ++ 12 files changed, 470 insertions(+), 80 deletions(-)