From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status Date: Tue, 19 Jun 2018 09:34:28 -0600 Message-ID: <2d6278d1-45ab-ff53-7b97-d9593203ff3e@gmail.com> References: <20180617151819.6824-1-dsahern@kernel.org> <20180618181123.eczjeb3axd6sao57@kafai-mbp.dhcp.thefacebook.com> <780331bd-947a-83fe-6e62-c0efc05cfc04@gmail.com> <20180618205537.2j645mfujdsqxf2b@kafai-mbp.dhcp.thefacebook.com> <1339f6f2-9dd3-886c-2178-7088b0ae4746@gmail.com> <20180619152529.rkzeyyqgmiwsvjp6@kafai-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: dsahern@kernel.org, netdev@vger.kernel.org, borkmann@iogearbox.net, ast@kernel.org, davem@davemloft.net To: Martin KaFai Lau Return-path: Received: from mail-pg0-f66.google.com ([74.125.83.66]:33475 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966078AbeFSPec (ORCPT ); Tue, 19 Jun 2018 11:34:32 -0400 Received: by mail-pg0-f66.google.com with SMTP id e11-v6so42739pgq.0 for ; Tue, 19 Jun 2018 08:34:31 -0700 (PDT) In-Reply-To: <20180619152529.rkzeyyqgmiwsvjp6@kafai-mbp.dhcp.thefacebook.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 6/19/18 9:25 AM, Martin KaFai Lau wrote: > 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. > I did consider returning the status in struct bpf_fib_lookup. However, it is 64 bytes and can not be extended without a big performance penalty, so the only option there is to make an existing entry a union the most logical of which is the ifindex. It seemed odd to me to have the result by hidden in the struct as a union on ifindex and returning the egress index from the function: @@ -2625,7 +2636,11 @@ struct bpf_fib_lookup { /* total length of packet from network header - used for MTU check */ __u16 tot_len; - __u32 ifindex; /* L3 device index for lookup */ + + union { + __u32 ifindex; /* input: L3 device index for lookup */ + __u32 result; /* output: one of BPF_FIB_LKUP_RET_* */ + }; It seemed more natural to have ifindex stay ifindex and only change value on return: @@ -2625,7 +2639,11 @@ struct bpf_fib_lookup { /* total length of packet from network header - used for MTU check */ __u16 tot_len; - __u32 ifindex; /* L3 device index for lookup */ + + /* input: L3 device index for lookup + * output: nexthop device index from FIB lookup + */ + __u32 ifindex; union { /* inputs to lookup */ >>From a program's perspective: rc < 0 -- program is passing incorrect data rc == 0 -- packet can be forwarded rc > 0 -- packet can not be forwarded. BPF programs are not required to track the LKUP_RET values any more than a function returning multiple negative values - the caller just checks rc < 0 means failure. If the program cares it can look at specific values of rc to see the specific value. The same applies with the LKUP_RET values - they are there to provide insight into why the packet is not forwarded directly if the program cares to know why.