netdev - Re: [PATCH bpf-net] bpf: Change bpf_fib

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: <20180618205537.2j645mfujdsqxf2b@kafai-mbp.dhcp.thefacebook.com>
Date:   Mon, 18 Jun 2018 13:55:37 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     David Ahern <dsahern@...il.com>
CC:     <dsahern@...nel.org>, <netdev@...r.kernel.org>,
        <borkmann@...earbox.net>, <ast@...nel.org>, <davem@...emloft.net>
Subject: Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup
 status

On Mon, Jun 18, 2018 at 12:27:07PM -0600, David Ahern wrote:
> On 6/18/18 12:11 PM, Martin KaFai Lau wrote:
> > On Sun, Jun 17, 2018 at 08:18:19AM -0700, dsahern@...nel.org wrote:
> >> From: David Ahern <dsahern@...il.com>
> >>
> >> For ACLs implemented using either FIB rules or FIB entries, the BPF
> >> program needs the FIB lookup status to be able to drop the packet.
> > Except BPF_FIB_LKUP_RET_SUCCESS and BPF_FIB_LKUP_RET_NO_NEIGH,  can you
> > give an example on how the xdp_prog may decide XDP_PASS vs XDP_DROP based
> > on other BPF_FIB_LKUP_RET_*?
> > 
> 
> 	rc = bpf_fib_lookup(ctx, &fib_params, sizeof(fib_params), flags);
> 	if (rc == 0)
> 		packet is forwarded, do the redirect
> 
> 	/* the program is misconfigured -- wrong parameters in struct or flags */
> 	if (rc < 0)
> 		....
> 
> 	/* rc > 0 case */
> 	switch(rc) {
> 	case BPF_FIB_LKUP_RET_BLACKHOLE:
> 	case BPF_FIB_LKUP_RET_UNREACHABLE:
> 	case BPF_FIB_LKUP_RET_PROHIBIT:
> 		return XDP_DROP;
> 	}
> 
> For the others it becomes a question of do we share why the stack needs
> to be involved? Maybe the program wants to collect stats to show traffic
> patterns that can be improved (BPF_FIB_LKUP_RET_FRAG_NEEDED) or support
> in the kernel needs to be improved (BPF_FIB_LKUP_RET_UNSUPP_LWT) or an
> interface is misconfigured (BPF_FIB_LKUP_RET_FWD_DISABLED).
Thanks for the explanation.

Agree on the bpf able to collect stats will be useful.

I am wondering, if a new BPF_FIB_LKUP_RET_XYZ is added later,
how may the old xdp_prog work/not-work?  As of now, the return value
is straight forward, FWD, PASS (to stack) or DROP (error).
With this change, the xdp_prog needs to match/switch() the
BPF_FIB_LKUP_RET_* to at least PASS and DROP.

> 
> Arguably BPF_FIB_LKUP_RET_NO_NHDEV is not needed. See below.
> 
> >> @@ -2612,6 +2613,19 @@ struct bpf_raw_tracepoint_args {
> >>  #define BPF_FIB_LOOKUP_DIRECT  BIT(0)
> >>  #define BPF_FIB_LOOKUP_OUTPUT  BIT(1)
> >>  
> >> +enum {
> >> +	BPF_FIB_LKUP_RET_SUCCESS,      /* lookup successful */
> >> +	BPF_FIB_LKUP_RET_BLACKHOLE,    /* dest is blackholed */
> >> +	BPF_FIB_LKUP_RET_UNREACHABLE,  /* dest is unreachable */
> >> +	BPF_FIB_LKUP_RET_PROHIBIT,     /* dest not allowed */
> >> +	BPF_FIB_LKUP_RET_NOT_FWDED,    /* pkt is not forwardded */
> > BPF_FIB_LKUP_RET_NOT_FWDED is a catch all?
> > 
> 
> Destination is local. More precisely, the FIB lookup is not unicast so
> not forwarded. It could be RTN_LOCAL, RTN_BROADCAST, RTN_ANYCAST, or
> RTN_MULTICAST. The next ones -- blackhole, reachable, prohibit -- are
> called out.
I think it also includes the tbid not found case.

> 
> >> @@ -4252,16 +4277,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
> >>  	if (check_mtu) {
> >>  		mtu = ipv6_stub->ip6_mtu_from_fib6(f6i, dst, src);
> >>  		if (params->tot_len > mtu)
> >> -			return 0;
> >> +			return BPF_FIB_LKUP_RET_FRAG_NEEDED;
> >>  	}
> >>  
> >>  	if (f6i->fib6_nh.nh_lwtstate)
> >> -		return 0;
> >> +		return BPF_FIB_LKUP_RET_UNSUPP_LWT;
> >>  
> >>  	if (f6i->fib6_flags & RTF_GATEWAY)
> >>  		*dst = f6i->fib6_nh.nh_gw;
> >>  
> >>  	dev = f6i->fib6_nh.nh_dev;
> >> +	if (unlikely(!dev))
> >> +		return BPF_FIB_LKUP_RET_NO_NHDEV;
> > Is this a bug fix?
> > 
> 
> Difference between IPv4 and IPv6. Making them consistent.
> 
> It is a major BUG in the kernel to reach this point in either protocol
> to have a unicast route not tied to a device. IPv4 has checks; v6 does
> not. I figured this being new code, why not make bpf_ipv{4,6}_fib_lookup
> as close to the same as possible.
Make sense.  A comment in the commit log will be useful if there is a
re-spin.