From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH bpf-next 3/3] bpf: Add mtu checking to FIB forwarding helper Date: Thu, 17 May 2018 18:34:22 -0600 Message-ID: References: <20180517160930.25076-1-dsahern@gmail.com> <20180517160930.25076-4-dsahern@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net To: Daniel Borkmann , netdev@vger.kernel.org, borkmann@iogearbox.net, ast@kernel.org Return-path: Received: from mail-io0-f196.google.com ([209.85.223.196]:44150 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751564AbeERAeZ (ORCPT ); Thu, 17 May 2018 20:34:25 -0400 Received: by mail-io0-f196.google.com with SMTP id d11-v6so4213478iof.11 for ; Thu, 17 May 2018 17:34:25 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 5/17/18 4:22 PM, Daniel Borkmann wrote: > On 05/17/2018 06:09 PM, David Ahern wrote: >> Add check that egress MTU can handle packet to be forwarded. If >> the MTU is less than the packet lenght, return 0 meaning the >> packet is expected to continue up the stack for help - eg., >> fragmenting the packet or sending an ICMP. >> >> Signed-off-by: David Ahern >> --- >> net/core/filter.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 6d0d1560bd70..c47c47a75d4b 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -4098,6 +4098,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params, >> struct fib_nh *nh; >> struct flowi4 fl4; >> int err; >> + u32 mtu; >> >> dev = dev_get_by_index_rcu(net, params->ifindex); >> if (unlikely(!dev)) >> @@ -4149,6 +4150,10 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params, >> if (res.fi->fib_nhs > 1) >> fib_select_path(net, &res, &fl4, NULL); >> >> + mtu = ip_mtu_from_fib_result(&res, params->ipv4_dst); >> + if (params->tot_len > mtu) >> + return 0; >> + >> nh = &res.fi->fib_nh[res.nh_sel]; >> >> /* do not handle lwt encaps right now */ >> @@ -4188,6 +4193,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params, >> struct flowi6 fl6; >> int strict = 0; >> int oif; >> + u32 mtu; >> >> /* link local addresses are never forwarded */ >> if (rt6_need_strict(dst) || rt6_need_strict(src)) >> @@ -4250,6 +4256,10 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params, >> fl6.flowi6_oif, NULL, >> strict); >> >> + mtu = ip6_mtu_from_fib6(f6i, dst, src); >> + if (params->tot_len > mtu) >> + return 0; >> + >> if (f6i->fib6_nh.nh_lwtstate) >> return 0; > > Could you elaborate how this interacts in tc BPF use case where you have e.g. > GSO packets and tot_len from aggregated packets would definitely be larger > than MTU (e.g. see is_skb_forwardable() as one example on such checks)? Should > this be an opt-in via a new flag for the helper? It should not be opt-in for XDP. I could add a flag to the internal call -- bpf_skb_fib_lookup sets the flag to skip the MTU check in bpf_ipv4_fib_lookup and bpf_ipv6_fib_lookup. For the skb case do you want bpf_skb_fib_lookup call is_skb_forwardable or leave that to the BPF program?