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
| ||
|
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 >>
Powered by blists - more mailing lists