From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH v2 bpf-net] bpf: Change bpf_fib_lookup to return lookup
 status
Date: Tue, 26 Jun 2018 07:53:41 -0600
Message-ID: <f33d42e4-ec29-bb5a-967e-dbacbd8ec076@gmail.com>
References: <20180621030011.7441-1-dsahern@kernel.org>
 <4cd62659-1b52-6746-5a37-faa01d476476@iogearbox.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: davem@davemloft.net, kafai@fb.com
To: Daniel Borkmann <daniel@iogearbox.net>, dsahern@kernel.org,
        netdev@vger.kernel.org, borkmann@iogearbox.net, ast@kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-io0-f196.google.com ([209.85.223.196]:41978 "EHLO
        mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S935437AbeFZNxo (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 26 Jun 2018 09:53:44 -0400
Received: by mail-io0-f196.google.com with SMTP id k16-v6so13285268ioa.8
        for <netdev@vger.kernel.org>; Tue, 26 Jun 2018 06:53:44 -0700 (PDT)
In-Reply-To: <4cd62659-1b52-6746-5a37-faa01d476476@iogearbox.net>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 6/26/18 3:50 AM, Daniel Borkmann wrote:

> [...]
> You change all the semantics of return code here, but this breaks bpf_skb_fib_lookup().
> I cannot see how this would work in that case. The code does the following with the
> bpf_ipv{4,6}_fib_lookup() return code:
> 
> [...]
>         switch (params->family) {
> #if IS_ENABLED(CONFIG_INET)
>         case AF_INET:
>                 index = bpf_ipv4_fib_lookup(net, params, flags, false);
>                 break;
> #endif
> #if IS_ENABLED(CONFIG_IPV6)
>         case AF_INET6:
>                 index = bpf_ipv6_fib_lookup(net, params, flags, false);
>                 break;
> #endif
>         }
> 
>         if (index > 0) {
>                 struct net_device *dev;
> 
>                 dev = dev_get_by_index_rcu(net, index);
>                 if (!is_skb_forwardable(dev, skb))
>                         index = 0;
>         }

Yes, I forgot to update the skb path. That should be rc now and then the
dev lookup based on params->ifindex. Will fix.

> [...]
> 
> So the BPF_FIB_LKUP_* results become the dev ifindex here and the !is_skb_forwardable()
> case further suggests that the packet *can* be forwarded based on the new semantics
> whereas MTU check is bypassed on success.
> 
> It probably helps to craft a selftest for XDP *and* tc case in future, so we can be sure
> nothing breaks with new changes.

yes, will do.