From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 08720C6369E for ; Wed, 2 Dec 2020 21:45:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 86814217A0 for ; Wed, 2 Dec 2020 21:45:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727084AbgLBVpU (ORCPT ); Wed, 2 Dec 2020 16:45:20 -0500 Received: from www62.your-server.de ([213.133.104.62]:37960 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726880AbgLBVpU (ORCPT ); Wed, 2 Dec 2020 16:45:20 -0500 Received: from sslproxy01.your-server.de ([78.46.139.224]) by www62.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92.3) (envelope-from ) id 1kkZvS-0003YO-17; Wed, 02 Dec 2020 22:44:38 +0100 Received: from [85.7.101.30] (helo=pc-9.home) by sslproxy01.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kkZvR-00036D-NS; Wed, 02 Dec 2020 22:44:37 +0100 Subject: Re: [PATCH bpf-next V7 2/8] bpf: fix bpf_fib_lookup helper MTU check for SKB ctx To: Jesper Dangaard Brouer , bpf@vger.kernel.org Cc: netdev@vger.kernel.org, Daniel Borkmann , Alexei Starovoitov , maze@google.com, lmb@cloudflare.com, shaun@tigera.io, Lorenzo Bianconi , marek@cloudflare.com, John Fastabend , Jakub Kicinski , eyal.birger@gmail.com, colrack@gmail.com References: <160588903254.2817268.4861837335793475314.stgit@firesoul> <160588909693.2817268.17116187979657760922.stgit@firesoul> From: Daniel Borkmann Message-ID: <6f9cac4e-a231-ff8d-43a5-828995ca5ec7@iogearbox.net> Date: Wed, 2 Dec 2020 22:44:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <160588909693.2817268.17116187979657760922.stgit@firesoul> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.102.4/26006/Wed Dec 2 14:14:18 2020) Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 11/20/20 5:18 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()). > > Fix this by using fib_params 'tot_len' for MTU check. If not provided > (e.g. zero) then keep existing behaviour intact. > > Fixes: 4c79579b44b1 ("bpf: Change bpf_fib_lookup to return lookup status") > Reported-by: Carlo Carraro > Signed-off-by: Jesper Dangaard Brouer > --- > net/core/filter.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 1ee97fdeea64..84d77c425fbe 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5565,11 +5565,21 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > #endif > } > > - if (!rc) { > + if (rc == BPF_FIB_LKUP_RET_SUCCESS) { > struct net_device *dev; > + u32 mtu; > > dev = dev_get_by_index_rcu(net, params->ifindex); > - if (!is_skb_forwardable(dev, skb)) > + mtu = READ_ONCE(dev->mtu); > + > + /* Using tot_len for (L3) MTU check if provided by user */ > + if (params->tot_len && params->tot_len > mtu) > + rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; Is there a reason why we cannot reuse and at the same time optimize the built-in bpf_ipv{4,6}_fib_lookup() check_mtu as we do in XDP when params->tot_len was specified.. something as presented earlier [0]? My biggest concern for gso skbs is that the above might be subject to breakage from one kernel version to another if it was filled from the packet. So going back and building upon [0], to be on safe side we could have a new flag like below to indicate wanted behavior: diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index c3458ec1f30a..d3cd2f47f011 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -4934,8 +4934,9 @@ struct bpf_raw_tracepoint_args { * OUTPUT: Do lookup from egress perspective; default is ingress */ enum { - BPF_FIB_LOOKUP_DIRECT = (1U << 0), - BPF_FIB_LOOKUP_OUTPUT = (1U << 1), + BPF_FIB_LOOKUP_DIRECT = (1U << 0), + BPF_FIB_LOOKUP_OUTPUT = (1U << 1), + BPF_FIB_LOOKUP_ALWAYS_MTU_CHECK = (1U << 2), }; enum { diff --git a/net/core/filter.c b/net/core/filter.c index 2ca5eecebacf..4fb876ebd6a0 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5547,37 +5547,27 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, struct bpf_fib_lookup *, params, int, plen, u32, flags) { - struct net *net = dev_net(skb->dev); - int rc = -EAFNOSUPPORT; + bool check_mtu = !skb_is_gso(skb) || + (flags & BPF_FIB_LOOKUP_ALWAYS_MTU_CHECK); if (plen < sizeof(*params)) return -EINVAL; - if (flags & ~(BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT)) + if (flags & ~(BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_OUTPUT | + BPF_FIB_LOOKUP_ALWAYS_MTU_CHECK)) return -EINVAL; switch (params->family) { #if IS_ENABLED(CONFIG_INET) case AF_INET: - rc = bpf_ipv4_fib_lookup(net, params, flags, false); - break; + return bpf_ipv4_fib_lookup(net, params, flags, check_mtu); #endif #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: - rc = bpf_ipv6_fib_lookup(net, params, flags, false); - break; + return bpf_ipv6_fib_lookup(net, params, flags, check_mtu); #endif } - - if (!rc) { - struct net_device *dev; - - dev = dev_get_by_index_rcu(net, params->ifindex); - if (!is_skb_forwardable(dev, skb)) - rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; - } - - return rc; + return -EAFNOSUPPORT; } static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { [0] https://lore.kernel.org/bpf/65d8f988-5b41-24c2-8501-7cbbddb1238e@iogearbox.net/ > + /* Notice at this TC cls_bpf level skb->len contains L2 size, > + * but is_skb_forwardable takes that into account > + */ > + if (params->tot_len == 0 && !is_skb_forwardable(dev, skb)) > rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; > } > > >