On Thu, 9 May 2019 at 20:36, Alexei Starovoitov <a...@fb.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.le...@gmail.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.le...@gmail.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!
