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.
