| Message ID | 160208770557.798237.11181325462593441941.stgit@firesoul |
|---|---|
| Headers | show |
| Series | bpf: New approach for BPF MTU handling and enforcement | expand |
On 10/7/20 6:22 PM, Jesper Dangaard Brouer wrote: [...] > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > (imported from commit 37f8552786cf46588af52b77829b730dd14524d3) slipped in?
> static u32 __bpf_skb_max_len(const struct sk_buff *skb) > { > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > - SKB_MAX_ALLOC; > + return IP_MAX_MTU; > } Shouldn't we just delete this helper instead and replace call sites?
Maciej Żenczykowski wrote: > On Wed, Oct 7, 2020 at 3:37 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > Daniel Borkmann wrote: > > > On 10/7/20 6:23 PM, Jesper Dangaard Brouer wrote: > > > [...] > > > > net/core/dev.c | 24 ++++++++++++++++++++++-- > > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > Couple high-level comments. Whats the problem with just letting the driver > > consume the packet? I would chalk it up to a buggy BPF program that is > > sending these packets. The drivers really shouldn't panic or do anything > > horrible under this case because even today I don't think we can be > > 100% certain MTU on skb matches set MTU. Imagine the case where I change > > the MTU from 9kB->1500B there will be some skbs in-flight with the larger > > length and some with the shorter. If the drivers panic/fault or otherwise > > does something else horrible thats not going to be friendly in general case > > regardless of what BPF does. And seeing this type of config is all done > > async its tricky (not practical) to flush any skbs in-flight. > > > > I've spent many hours debugging these types of feature flag, mtu > > change bugs on the driver side I'm not sure it can be resolved by > > the stack easily. Better to just build drivers that can handle it IMO. > > > > Do we know if sending >MTU size skbs to drivers causes problems in real > > cases? I haven't tried on the NICs I have here, but I expect they should > > be fine. Fine here being system keeps running as expected. Dropping the > > skb either on TX or RX side is expected. Even with this change though > > its possible for the skb to slip through if I configure MTU on a live > > system. > > I wholeheartedly agree with the above. > > Ideally the only >mtu check should happen at driver admittance. > But again ideally it should happen in some core stack location not in > the driver itself. Ideally maybe, but IMO we should just let the skb go to the driver and let the driver sort it out. Even if this means pushing the packet onto the wire then the switch will drop it or the receiver, etc. A BPF program can do lots of horrible things that should never be on the wire otherwise. MTU is just one of them, but sending corrupted payloads, adding bogus headers, checksums etc. so I don't think we can reasonable protect against all of them. Of course if the driver is going to hang/panic then something needs to be done. Perhaps a needs_mtu_check feature flag, although thats not so nice either so perhaps drivers just need to handle it themselves. Also even today the case could happen without BPF as best I can tell so the drivers should be prepared for it. > However, due to both gso and vlan offload, even this is not trivial to do... > The mtu is L3, but drivers/hardware/the wire usually care about L2... > (because ultimately that's what gets received and must fit in receive buffers)
On Wed, 07 Oct 2020 20:19:39 -0700 John Fastabend <john.fastabend@gmail.com> wrote: > Maciej Żenczykowski wrote: > > On Wed, Oct 7, 2020 at 3:37 PM John Fastabend <john.fastabend@gmail.com> wrote: > > > > > > Daniel Borkmann wrote: > > > > On 10/7/20 6:23 PM, Jesper Dangaard Brouer wrote: > > > > [...] > > > > > net/core/dev.c | 24 ++++++++++++++++++++++-- > > > > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > > > Couple high-level comments. Whats the problem with just letting the driver > > > consume the packet? I would chalk it up to a buggy BPF program that is > > > sending these packets. The drivers really shouldn't panic or do anything > > > horrible under this case because even today I don't think we can be > > > 100% certain MTU on skb matches set MTU. Imagine the case where I change > > > the MTU from 9kB->1500B there will be some skbs in-flight with the larger > > > length and some with the shorter. If the drivers panic/fault or otherwise > > > does something else horrible thats not going to be friendly in general case > > > regardless of what BPF does. And seeing this type of config is all done > > > async its tricky (not practical) to flush any skbs in-flight. > > > > > > I've spent many hours debugging these types of feature flag, mtu > > > change bugs on the driver side I'm not sure it can be resolved by > > > the stack easily. Better to just build drivers that can handle it IMO. > > > > > > Do we know if sending >MTU size skbs to drivers causes problems in real > > > cases? I haven't tried on the NICs I have here, but I expect they should > > > be fine. Fine here being system keeps running as expected. Dropping the > > > skb either on TX or RX side is expected. Even with this change though > > > its possible for the skb to slip through if I configure MTU on a live > > > system. > > > > I wholeheartedly agree with the above. > > > > Ideally the only >mtu check should happen at driver admittance. > > But again ideally it should happen in some core stack location not in > > the driver itself. > > Ideally maybe, but IMO we should just let the skb go to the driver > and let the driver sort it out. Even if this means pushing the packet > onto the wire then the switch will drop it or the receiver, etc. A > BPF program can do lots of horrible things that should never be > on the wire otherwise. MTU is just one of them, but sending corrupted > payloads, adding bogus headers, checksums etc. so I don't think we can > reasonable protect against all of them. That is a good point. > Of course if the driver is going to hang/panic then something needs > to be done. Perhaps a needs_mtu_check feature flag, although thats > not so nice either so perhaps drivers just need to handle it themselves. > Also even today the case could happen without BPF as best I can tell > so the drivers should be prepared for it. Yes, borderline cases do exist already today (like your reconf with inflight packets), so I guess drivers should at-least not hang/panic and be robust enough that we can drop this check. I think you have convinced me that we can drop this check. My only concern is how people can troubleshoot this, but its not different from current state. > > However, due to both gso and vlan offload, even this is not trivial to do... > > The mtu is L3, but drivers/hardware/the wire usually care about L2... If net_device->mtu is L3 (1500) and XDP (and TC, right?) operate at L2, that likely means that the "strict" bpf_mtu_check (in my BPF-helper) is wrong, as XDP (and TC) length at this point include the 14 bytes Ethernet header. I will check and fix. Is this accounted for via net_device->hard_header_len ?
On 10/8/20 10:07 AM, Jesper Dangaard Brouer wrote: [...] >>> However, due to both gso and vlan offload, even this is not trivial to do... >>> The mtu is L3, but drivers/hardware/the wire usually care about L2... > > If net_device->mtu is L3 (1500) and XDP (and TC, right?) operate at L2, > that likely means that the "strict" bpf_mtu_check (in my BPF-helper) is > wrong, as XDP (and TC) length at this point include the 14 bytes > Ethernet header. I will check and fix. Yes, both at L2 layer. > Is this accounted for via net_device->hard_header_len ? It is, see also ether_setup().
On Wed, 7 Oct 2020 23:37:00 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 10/7/20 6:23 PM, Jesper Dangaard Brouer wrote: > [...] > > net/core/dev.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/dev.c b/net/core/dev.c > > index b433098896b2..19406013f93e 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -3870,6 +3870,7 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev) > > switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) { > > case TC_ACT_OK: > > case TC_ACT_RECLASSIFY: > > + *ret = NET_XMIT_SUCCESS; > > skb->tc_index = TC_H_MIN(cl_res.classid); > > break; > > case TC_ACT_SHOT: > > @@ -4064,9 +4065,12 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > { > > struct net_device *dev = skb->dev; > > struct netdev_queue *txq; > > +#ifdef CONFIG_NET_CLS_ACT > > + bool mtu_check = false; > > +#endif > > + bool again = false; > > struct Qdisc *q; > > int rc = -ENOMEM; > > - bool again = false; > > > > skb_reset_mac_header(skb); > > > > @@ -4082,14 +4086,28 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > > > qdisc_pkt_len_init(skb); > > #ifdef CONFIG_NET_CLS_ACT > > + mtu_check = skb_is_redirected(skb); > > skb->tc_at_ingress = 0; > > # ifdef CONFIG_NET_EGRESS > > if (static_branch_unlikely(&egress_needed_key)) { > > + unsigned int len_orig = skb->len; > > + > > skb = sch_handle_egress(skb, &rc, dev); > > if (!skb) > > goto out; > > + /* BPF-prog ran and could have changed packet size beyond MTU */ > > + if (rc == NET_XMIT_SUCCESS && skb->len > len_orig) > > + mtu_check = true; > > } > > # endif > > + /* MTU-check only happens on "last" net_device in a redirect sequence > > + * (e.g. above sch_handle_egress can steal SKB and skb_do_redirect it > > + * either ingress or egress to another device). > > + */ > > Hmm, quite some overhead in fast path. Not really, normal fast-path already call is_skb_forwardable(). And it already happens in existing code, ingress redirect code, which I remove calling in patch 6. (I have considered inlining is_skb_forwardable as a optimization for general netstack dev_forward_skb) > Also, won't this be checked multiple times on stacked devices? :( I don't think it will be checked multiple times, because we have a skb_reset_redirect() in ingress path (just after sch_handle_ingress()). > Moreover, this missed the fact that 'real' qdiscs can have > filters attached too, this would come after this check. Can't this instead be in > driver layer for those that really need it? I would probably only drop the check > as done in 1/6 and allow the BPF prog to do the validation if needed. See other reply, this is likely what we will end-up with. > > + if (mtu_check && !is_skb_forwardable(dev, skb)) { > > + rc = -EMSGSIZE; > > + goto drop; > > + } > > #endif > > /* If device/qdisc don't need skb->dst, release it right now while > > * its hot in this cpu cache. > > @@ -4157,7 +4175,9 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > > > rc = -ENETDOWN; > > rcu_read_unlock_bh(); > > - > > +#ifdef CONFIG_NET_CLS_ACT > > +drop: > > +#endif > > atomic_long_inc(&dev->tx_dropped); > > kfree_skb_list(skb); > > return rc; > > >
On Wed, 7 Oct 2020 16:46:10 -0700 Maciej Żenczykowski <maze@google.com> wrote: > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > { > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > - SKB_MAX_ALLOC; > > + return IP_MAX_MTU; > > } > > Shouldn't we just delete this helper instead and replace call sites? It does seem wrong to pass argument skb into this function, as it is no-longer used... Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU.
On Thu, Oct 8, 2020 at 7:06 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Wed, 7 Oct 2020 16:46:10 -0700 > Maciej Żenczykowski <maze@google.com> wrote: > > > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > > { > > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > > - SKB_MAX_ALLOC; > > > + return IP_MAX_MTU; > > > } > > > > Shouldn't we just delete this helper instead and replace call sites? > > It does seem wrong to pass argument skb into this function, as it is > no-longer used... > > Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU. Should that be IP6_MAX_MTU, which is larger than IP_MAX_MTU?
On Thu, 8 Oct 2020 08:33:04 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Thu, Oct 8, 2020 at 7:06 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > > > On Wed, 7 Oct 2020 16:46:10 -0700 > > Maciej Żenczykowski <maze@google.com> wrote: > > > > > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > > > { > > > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > > > - SKB_MAX_ALLOC; > > > > + return IP_MAX_MTU; > > > > } > > > > > > Shouldn't we just delete this helper instead and replace call sites? > > > > It does seem wrong to pass argument skb into this function, as it is > > no-longer used... > > > > Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU. > > Should that be IP6_MAX_MTU, which is larger than IP_MAX_MTU? Sure I'll do that, and handle that is hides under CONFIG_IPV6.
This patchset drops all the MTU checks in TC BPF-helpers that limits growing the packet size. This is done because these BPF-helpers doesn't take redirect into account, which can result in their MTU check being done against the wrong netdev. The new approach is to give BPF-programs knowledge about the MTU on a netdev (via ifindex) and fib route lookup level. Meaning some BPF-helpers are added and extended to make it possible to do MTU checks in the BPF-code. If BPF-prog doesn't comply with the MTU this is enforced on the kernel side. Realizing MTU should only apply to transmitted packets, the MTU enforcement is now done after the TC egress hook. This gives TC-BPF programs most flexibility and allows to shrink packet size again in egress hook prior to transmit. This patchset is primarily focused on TC-BPF, but I've made sure that the MTU BPF-helpers also works for XDP BPF-programs. V2: New BPF-helper design --- Jesper Dangaard Brouer (6): bpf: Remove MTU check in __bpf_skb_max_len bpf: bpf_fib_lookup return MTU value as output when looked up bpf: add BPF-helper for MTU checking bpf: make it possible to identify BPF redirected SKBs bpf: Add MTU check for TC-BPF packets after egress hook bpf: drop MTU check when doing TC-BPF redirect to ingress include/linux/netdevice.h | 5 +- include/uapi/linux/bpf.h | 68 ++++++++++++++++++++- net/core/dev.c | 28 ++++++++ net/core/filter.c | 149 ++++++++++++++++++++++++++++++++++++++++++--- net/sched/Kconfig | 1 5 files changed, 235 insertions(+), 16 deletions(-) --