Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status

[David Ahern <dsahern@gmail.com>]

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.

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