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