netdev - Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9a1a0d6a-d7d4-406a-6bad-26f222df073f@fb.com>
Date:   Thu, 9 May 2019 18:36:20 +0000
From:   Alexei Starovoitov <ast@...com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>,
        Björn Töpel <bjorn.topel@...il.com>
CC:     Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an
 xskmap

On 5/9/19 9:12 AM, Jonathan Lemon wrote:
> On 9 May 2019, at 4:48, Björn Töpel wrote:
> 
>> On Thu, 9 May 2019 at 01:07, Jonathan Lemon <jonathan.lemon@...il.com> 
>> wrote:
>>>
>>> Currently, the AF_XDP code uses a separate map in order to
>>> determine if an xsk is bound to a queue.  Instead of doing this,
>>> have bpf_map_lookup_elem() return a boolean indicating whether
>>> there is a valid entry at the map index.
>>>
>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
>>> ---
>>>  kernel/bpf/verifier.c                             |  6 +++++-
>>>  kernel/bpf/xskmap.c                               |  2 +-
>>>  .../selftests/bpf/verifier/prevent_map_lookup.c   | 15 ---------------
>>>  3 files changed, 6 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 7b05e8938d5c..a8b8ff9ecd90 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -2761,10 +2761,14 @@ static int 
>>> check_map_func_compatibility(struct bpf_verifier_env *env,
>>>          * appear.
>>>          */
>>>         case BPF_MAP_TYPE_CPUMAP:
>>> -       case BPF_MAP_TYPE_XSKMAP:
>>>                 if (func_id != BPF_FUNC_redirect_map)
>>>                         goto error;
>>>                 break;
>>> +       case BPF_MAP_TYPE_XSKMAP:
>>> +               if (func_id != BPF_FUNC_redirect_map &&
>>> +                   func_id != BPF_FUNC_map_lookup_elem)
>>> +                       goto error;
>>> +               break;
>>>         case BPF_MAP_TYPE_ARRAY_OF_MAPS:
>>>         case BPF_MAP_TYPE_HASH_OF_MAPS:
>>>                 if (func_id != BPF_FUNC_map_lookup_elem)
>>> diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
>>> index 686d244e798d..f6e49237979c 100644
>>> --- a/kernel/bpf/xskmap.c
>>> +++ b/kernel/bpf/xskmap.c
>>> @@ -154,7 +154,7 @@ void __xsk_map_flush(struct bpf_map *map)
>>>
>>>  static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
>>>  {
>>> -       return ERR_PTR(-EOPNOTSUPP);
>>> +       return !!__xsk_map_lookup_elem(map, *(u32 *)key);
>>>  }
>>>
>>
>> Hmm, enabling lookups has some concerns, so we took the easy path;
>> simply disallowing it. Lookups (and returning a socket/fd) from
>> userspace might be expensive; allocating a new fd, and such, and on
>> the BPF side there's no XDP socket object (yet!).
>>
>> Your patch makes the lookup return something else than a fd or socket.
>> The broader question is, inserting a socket fd and getting back a bool
>> -- is that ok from a semantic perspective? It's a kind of weird map.
>> Are there any other maps that behave in this way? It certainly makes
>> the XDP code easier, and you get somewhat better introspection into
>> the XSKMAP.
> 
> I simply want to query the map and ask "is there an entry present?",
> but there isn't a separate API for that.  It seems really odd that I'm
> required to duplicate the same logic by using a second map.  I agree that
> there isn't any point in returning an fd or xdp socket object - hence
> the boolean.
> 
> The comment inthe verifier does read:
> 
>          /* Restrict bpf side of cpumap and xskmap, open when use-cases
>           * appear.
> 
> so I'd say this is a use-case.  :)
> 
> The cpumap cpu_map_lookup_elem() function returns the qsize for some
> reason, but it doesn't seem reachable from the verifier.

I think it's good to expose some info about xsk to bpf prog.
Returning bool is kinda single purpose.
Can xsk_map_lookup_elem() return xsk.sk.sk_cookie instead?
I think we can force non zero cookie for all xsk sockets
then returning zero would mean that socket is not there
and can solve this use case as well.
Or some other property of xsk ?

Probably better idea would be to return 'struct bpf_sock *' or
new 'struct bpf_xdp_sock *' and teach the verifier to extract
xsk.queue_id or other interesting info from it.
I think it's safe to do, since progs run under rcu.