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] [day] [month] [year] [list]

Date:   Mon, 13 May 2019 11:29:12 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Alexei Starovoitov <ast@...com>
Cc:     Jonathan Lemon <jonathan.lemon@...il.com>,
        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 Thu, 9 May 2019 at 20:36, Alexei Starovoitov <ast@...com> wrote:
>
> 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.

Good ideas, definitely worth trying out! ...and again, getting rid of
that extra map is very clean!