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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e8744809-4f99-5268-da89-0080dd5b3b85@iogearbox.net>
Date:   Fri, 18 May 2018 00:22:45 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org,
        borkmann@...earbox.net, ast@...nel.org
Cc:     davem@...emloft.net
Subject: Re: [PATCH bpf-next 3/3] bpf: Add mtu checking to FIB forwarding
 helper

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@...il.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?

Thanks,
Daniel