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]
Date:   Tue, 19 Jun 2018 08:25:29 -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 03:35:25PM -0600, David Ahern wrote:
> On 6/18/18 2:55 PM, Martin KaFai Lau wrote:
> >> 	/* 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.
> 
> IMO, programs should only call XDP_DROP for known reasons - like the 3
> above. Anything else punt to the stack.
> 
> If a new RET_XYZ comes along:
> 1. the new XYZ is a new ACL response where the packet is to be dropped.
> If the program does not understand XYZ and punts to the stack
> (recommendation), then a second lookup is done during normal packet
> processing and the stack drops it.
> 
> 2. the new XYZ is a new path in the kernel that is unsupported with
> respect to XDP forwarding, nothing new for the program to do.
> 
> Either way I would expect stats on BPF_FIB_LKUP_RET_* to give a hint to
> the program writer.
> 
> Worst case of punting packets to the stack for any rc != 0 means the
> stack is doing 2 lookups - 1 in XDP based on its lookup parameters and 1
> in normal stack processing - to handle the packet.
Instead of having the xdp_prog to follow the meaning of what RET_SYZ is,
should the bpf_*_fib_lookup() return value be kept as is such that
the xdp_prog is clear what to do.  The reason can be returned in
the 'struct bpf_fib_lookup'.  The number of reasons can be extended.
If the xdp_prog does not understand a reason, it still will not
affect its decision because the return value is clear.
I think the situation here is similar to regular syscall which usually
uses -1 to clearly states error and errno to spells out the reason.

> 
> > 
> >>
> >> 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.
> 
> Another one of those "should never happen scenarios". The user does not
> specify the table; it is retrieved based on device association. Table
> defaults to the main table - which always exists - and any VRF
> enslavement of a device happens after the VRF device creates the table.
> 
> > 
> >>
> >>>> @@ -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.
> > 
> 
> ok.