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.
(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
