On 6/18/18 12:11 PM, Martin KaFai Lau wrote:
> On Sun, Jun 17, 2018 at 08:18:19AM -0700, dsah...@kernel.org wrote:
>> From: David Ahern <dsah...@gmail.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).
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.
>> @@ -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.
