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: <7974B49C-CC4F-475D-992A-3E5B6480B039@gmail.com>
Date:   Thu, 09 May 2019 09:12:58 -0700
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "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@...com
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an
 xskmap

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.

>
> (bpf-next is closed, btw... :-))

Ah yes, thanks.
-- 
Jonathan



>
>
>
> Björn
>
>>  static int xsk_map_update_elem(struct bpf_map *map, void *key, void 
>> *value,
>> diff --git 
>> a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c 
>> b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
>> index bbdba990fefb..da7a4b37cb98 100644
>> --- a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
>> +++ b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c
>> @@ -28,21 +28,6 @@
>>         .errstr = "cannot pass map_type 18 into func 
>> bpf_map_lookup_elem",
>>         .prog_type = BPF_PROG_TYPE_SOCK_OPS,
>>  },
>> -{
>> -       "prevent map lookup in xskmap",
>> -       .insns = {
>> -       BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> -       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
>> -       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> -       BPF_LD_MAP_FD(BPF_REG_1, 0),
>> -       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, 
>> BPF_FUNC_map_lookup_elem),
>> -       BPF_EXIT_INSN(),
>> -       },
>> -       .fixup_map_xskmap = { 3 },
>> -       .result = REJECT,
>> -       .errstr = "cannot pass map_type 17 into func 
>> bpf_map_lookup_elem",
>> -       .prog_type = BPF_PROG_TYPE_XDP,
>> -},
>>  {
>>         "prevent map lookup in stack trace",
>>         .insns = {
>> --
>> 2.17.1
>>