Re: [PATCH bpf-next 3/3] bpf: Add mtu checking to FIB forwarding helper

[David Ahern <dsahern@gmail.com>]

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 <dsahern@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.

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?