| Message ID | 20190508225016.2375828-1-jonathan.lemon@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap | expand |
On Thu, 9 May 2019 at 01:07, Jonathan Lemon <jonathan.lemon@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.lemon@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. (bpf-next is closed, btw... :-)) 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 >
On 9 May 2019, at 4:48, Björn Töpel wrote: > On Thu, 9 May 2019 at 01:07, Jonathan Lemon <jonathan.lemon@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.lemon@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. > > (bpf-next is closed, btw... :-)) Ah yes, thanks.
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@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.lemon@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.
On Thu, 9 May 2019 at 20:36, Alexei Starovoitov <ast@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.lemon@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.lemon@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!
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); } 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 = {
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@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(-)