| Message ID | 161098885996.108067.14467274374916086727.stgit@firesoul (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: New approach for BPF MTU handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: yhs@fb.com ast@kernel.org kpsingh@kernel.org andrii@kernel.org daniel@iogearbox.net davem@davemloft.net dsahern@gmail.com kafai@fb.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 45 this patch: 45 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 38 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 45 this patch: 45 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 1/18/21 5:54 PM, Jesper Dangaard Brouer wrote: > BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use > bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size, > by adjusting fib_params 'tot_len' with the packet length plus the expected > encap size. (Just like the bpf_check_mtu helper supports). He discovered > that for SKB ctx the param->tot_len was not used, instead skb->len was used > (via MTU check in is_skb_forwardable() that checks against netdev MTU). > > Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g. > zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU > check is done like XDP code-path, which checks against FIB-dst MTU. > > V10: > - Use same method as XDP for 'tot_len' MTU check > > Fixes: 4c79579b44b1 ("bpf: Change bpf_fib_lookup to return lookup status") > Reported-by: Carlo Carraro <colrack@gmail.com> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > net/core/filter.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 5beadd659091..d5e6f395cf64 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5569,6 +5569,7 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > { > struct net *net = dev_net(skb->dev); > int rc = -EAFNOSUPPORT; > + bool check_mtu = false; > > if (plen < sizeof(*params)) > return -EINVAL; > @@ -5576,22 +5577,28 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > if (flags & ~(BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT)) > return -EINVAL; > > + if (params->tot_len) > + check_mtu = true; > + > switch (params->family) { > #if IS_ENABLED(CONFIG_INET) > case AF_INET: > - rc = bpf_ipv4_fib_lookup(net, params, flags, false); > + rc = bpf_ipv4_fib_lookup(net, params, flags, check_mtu); > break; > #endif > #if IS_ENABLED(CONFIG_IPV6) > case AF_INET6: > - rc = bpf_ipv6_fib_lookup(net, params, flags, false); > + rc = bpf_ipv6_fib_lookup(net, params, flags, check_mtu); > break; > #endif > } > > - if (!rc) { > + if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) { > struct net_device *dev; > > + /* When tot_len isn't provided by user, > + * check skb against net_device MTU > + */ > dev = dev_get_by_index_rcu(net, params->ifindex); > if (!is_skb_forwardable(dev, skb)) > rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; Btw, looking at some of the old feedback, looks like [0] got missed somehow. Would be nice if we could simplify this rather ugly bit with refetching dev for tc. [0] https://lore.kernel.org/bpf/f959017b-5d3c-5cdb-a016-c467a3c9a2fc@iogearbox.net/ https://lore.kernel.org/bpf/f8ff26f0-b1b6-6dd1-738d-4c592a8efdb0@gmail.com/
On Sat, 23 Jan 2021 02:47:28 +0100 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 1/18/21 5:54 PM, Jesper Dangaard Brouer wrote: > > BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use > > bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size, > > by adjusting fib_params 'tot_len' with the packet length plus the expected > > encap size. (Just like the bpf_check_mtu helper supports). He discovered > > that for SKB ctx the param->tot_len was not used, instead skb->len was used > > (via MTU check in is_skb_forwardable() that checks against netdev MTU). > > > > Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g. > > zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU > > check is done like XDP code-path, which checks against FIB-dst MTU. > > > > V10: > > - Use same method as XDP for 'tot_len' MTU check > > > > Fixes: 4c79579b44b1 ("bpf: Change bpf_fib_lookup to return lookup status") > > Reported-by: Carlo Carraro <colrack@gmail.com> > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > net/core/filter.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 5beadd659091..d5e6f395cf64 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -5569,6 +5569,7 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > > { > > struct net *net = dev_net(skb->dev); > > int rc = -EAFNOSUPPORT; > > + bool check_mtu = false; > > > > if (plen < sizeof(*params)) > > return -EINVAL; > > @@ -5576,22 +5577,28 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > > if (flags & ~(BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT)) > > return -EINVAL; > > > > + if (params->tot_len) > > + check_mtu = true; > > + > > switch (params->family) { > > #if IS_ENABLED(CONFIG_INET) > > case AF_INET: > > - rc = bpf_ipv4_fib_lookup(net, params, flags, false); > > + rc = bpf_ipv4_fib_lookup(net, params, flags, check_mtu); > > break; > > #endif > > #if IS_ENABLED(CONFIG_IPV6) > > case AF_INET6: > > - rc = bpf_ipv6_fib_lookup(net, params, flags, false); > > + rc = bpf_ipv6_fib_lookup(net, params, flags, check_mtu); > > break; > > #endif > > } > > > > - if (!rc) { > > + if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) { > > struct net_device *dev; > > > > + /* When tot_len isn't provided by user, > > + * check skb against net_device MTU > > + */ > > dev = dev_get_by_index_rcu(net, params->ifindex); > > if (!is_skb_forwardable(dev, skb)) > > rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; > > Btw, looking at some of the old feedback, looks like [0] got missed somehow. Would > be nice if we could simplify this rather ugly bit with refetching dev for tc. > > [0] https://lore.kernel.org/bpf/f959017b-5d3c-5cdb-a016-c467a3c9a2fc@iogearbox.net/ > https://lore.kernel.org/bpf/f8ff26f0-b1b6-6dd1-738d-4c592a8efdb0@gmail.com/ I have tried to incorporate the ideas from [0]. If you notice this code path is only called in-case params->tot_len is zero (!check_mtu). Then if params->tot_len does contain something, then the check_mtu code path of bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup(). I agree, that it is ugly refetching dev. I'll look at moving the lookup dev_get_by_index_rcu() call one level-up, as both bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() does this call. (I cannot move the check into the functions due skb is not avail in XDP case)
diff --git a/net/core/filter.c b/net/core/filter.c index 5beadd659091..d5e6f395cf64 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5569,6 +5569,7 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, { struct net *net = dev_net(skb->dev); int rc = -EAFNOSUPPORT; + bool check_mtu = false; if (plen < sizeof(*params)) return -EINVAL; @@ -5576,22 +5577,28 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, if (flags & ~(BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT)) return -EINVAL; + if (params->tot_len) + check_mtu = true; + switch (params->family) { #if IS_ENABLED(CONFIG_INET) case AF_INET: - rc = bpf_ipv4_fib_lookup(net, params, flags, false); + rc = bpf_ipv4_fib_lookup(net, params, flags, check_mtu); break; #endif #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: - rc = bpf_ipv6_fib_lookup(net, params, flags, false); + rc = bpf_ipv6_fib_lookup(net, params, flags, check_mtu); break; #endif } - if (!rc) { + if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) { struct net_device *dev; + /* When tot_len isn't provided by user, + * check skb against net_device MTU + */ dev = dev_get_by_index_rcu(net, params->ifindex); if (!is_skb_forwardable(dev, skb)) rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size, by adjusting fib_params 'tot_len' with the packet length plus the expected encap size. (Just like the bpf_check_mtu helper supports). He discovered that for SKB ctx the param->tot_len was not used, instead skb->len was used (via MTU check in is_skb_forwardable() that checks against netdev MTU). Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g. zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU check is done like XDP code-path, which checks against FIB-dst MTU. V10: - Use same method as XDP for 'tot_len' MTU check Fixes: 4c79579b44b1 ("bpf: Change bpf_fib_lookup to return lookup status") Reported-by: Carlo Carraro <colrack@gmail.com> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/core/filter.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)