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.
