On 05/18/2018 02:34 AM, David Ahern wrote: > 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 <dsah...@gmail.com> >>> --- >>> 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.
Yes, correct, for XDP it should not. > 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? I think it probably makes sense to add an internal (unexposed) flag or bool where we propagate skb_is_gso(skb) from bpf_skb_fib_lookup() call-site and have similar logic where we first check this bool and if false do the MTU check (so it still can get enforced for control packets). Thus probably nothing of that implementation detail needs to be exposed to the program author.
