* [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap
@ 2019-05-08 22:50 Jonathan Lemon
2019-05-08 22:50 ` [PATCH bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon
2019-05-09 11:48 ` [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Björn Töpel
0 siblings, 2 replies; 6+ messages in thread
From: Jonathan Lemon @ 2019-05-08 22:50 UTC (permalink / raw)
To: netdev, bjorn.topel, magnus.karlsson, ast; +Cc: kernel-team
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);
}
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs.
2019-05-08 22:50 [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon
@ 2019-05-08 22:50 ` Jonathan Lemon
2019-05-09 11:48 ` [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Björn Töpel
1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Lemon @ 2019-05-08 22:50 UTC (permalink / raw)
To: netdev, bjorn.topel, magnus.karlsson, ast; +Cc: kernel-team
Use the recent change to XSKMAP bpf_map_lookup_elem() to test if
there is a xsk present in the map instead of duplicating the work
with qidconf.
Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD
bypass any internal bpf maps, so xsk_socket__{create|delete} works
properly.
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
tools/lib/bpf/xsk.c | 79 +++++++++------------------------------------
1 file changed, 16 insertions(+), 63 deletions(-)
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index a3d1a302bc9c..470851090839 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -60,10 +60,8 @@ struct xsk_socket {
struct xsk_umem *umem;
struct xsk_socket_config config;
int fd;
- int xsks_map;
int ifindex;
int prog_fd;
- int qidconf_map_fd;
int xsks_map_fd;
__u32 queue_id;
char ifname[IFNAMSIZ];
@@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
/* This is the C-program:
* SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx)
* {
- * int *qidconf, index = ctx->rx_queue_index;
+ * int index = ctx->rx_queue_index;
*
* // A set entry here means that the correspnding queue_id
* // has an active AF_XDP socket bound to it.
- * qidconf = bpf_map_lookup_elem(&qidconf_map, &index);
- * if (!qidconf)
- * return XDP_ABORTED;
- *
- * if (*qidconf)
+ * if (bpf_map_lookup_elem(&xsks_map, &index))
* return bpf_redirect_map(&xsks_map, index, 0);
*
* return XDP_PASS;
@@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4),
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4),
- BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd),
+ BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd),
BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
- BPF_MOV32_IMM(BPF_REG_0, 0),
- /* if r1 == 0 goto +8 */
- BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8),
BPF_MOV32_IMM(BPF_REG_0, 2),
- /* r1 = *(u32 *)(r1 + 0) */
- BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
/* if r1 == 0 goto +5 */
BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5),
/* r2 = *(u32 *)(r10 - 4) */
@@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk)
if (max_queues < 0)
return max_queues;
- fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map",
+ fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "xsks_map",
sizeof(int), sizeof(int), max_queues, 0);
if (fd < 0)
return fd;
- xsk->qidconf_map_fd = fd;
- fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map",
- sizeof(int), sizeof(int), max_queues, 0);
- if (fd < 0) {
- close(xsk->qidconf_map_fd);
- return fd;
- }
xsk->xsks_map_fd = fd;
return 0;
@@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk)
static void xsk_delete_bpf_maps(struct xsk_socket *xsk)
{
- close(xsk->qidconf_map_fd);
+ bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id);
close(xsk->xsks_map_fd);
- xsk->qidconf_map_fd = -1;
- xsk->xsks_map_fd = -1;
}
static int xsk_lookup_bpf_maps(struct xsk_socket *xsk)
@@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk)
if (err)
goto out_map_ids;
- for (i = 0; i < prog_info.nr_map_ids; i++) {
- if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1)
- break;
+ xsk->xsks_map_fd = -1;
+ for (i = 0; i < prog_info.nr_map_ids; i++) {
fd = bpf_map_get_fd_by_id(map_ids[i]);
if (fd < 0)
continue;
@@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk)
continue;
}
- if (!strcmp(map_info.name, "qidconf_map")) {
- xsk->qidconf_map_fd = fd;
- continue;
- }
-
if (!strcmp(map_info.name, "xsks_map")) {
xsk->xsks_map_fd = fd;
continue;
@@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk)
}
err = 0;
- if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) {
+ if (xsk->xsks_map_fd == -1)
err = -ENOENT;
- xsk_delete_bpf_maps(xsk);
- }
out_map_ids:
free(map_ids);
return err;
}
-static void xsk_clear_bpf_maps(struct xsk_socket *xsk)
-{
- int qid = false;
-
- bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0);
- bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id);
-}
-
static int xsk_set_bpf_maps(struct xsk_socket *xsk)
{
- int qid = true, fd = xsk->fd, err;
-
- err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0);
- if (err)
- goto out;
-
- err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, 0);
- if (err)
- goto out;
-
- return 0;
-out:
- xsk_clear_bpf_maps(xsk);
- return err;
+ return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id,
+ &xsk->fd, 0);
}
static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
@@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
out_load:
close(xsk->prog_fd);
+ xsk->prog_fd = -1;
out_maps:
xsk_delete_bpf_maps(xsk);
return err;
@@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
goto out_mmap_tx;
}
- xsk->qidconf_map_fd = -1;
- xsk->xsks_map_fd = -1;
-
+ xsk->prog_fd = -1;
if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
err = xsk_setup_xdp_prog(xsk);
if (err)
@@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk)
if (!xsk)
return;
- xsk_clear_bpf_maps(xsk);
- xsk_delete_bpf_maps(xsk);
+ if (xsk->prog_fd != -1) {
+ xsk_delete_bpf_maps(xsk);
+ close(xsk->prog_fd);
+ }
optlen = sizeof(off);
err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap
2019-05-08 22:50 [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon
2019-05-08 22:50 ` [PATCH bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon
@ 2019-05-09 11:48 ` Björn Töpel
2019-05-09 16:12 ` Jonathan Lemon
1 sibling, 1 reply; 6+ messages in thread
From: Björn Töpel @ 2019-05-09 11:48 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Netdev, Björn Töpel, Karlsson, Magnus,
Alexei Starovoitov, kernel-team
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap
2019-05-09 11:48 ` [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Björn Töpel
@ 2019-05-09 16:12 ` Jonathan Lemon
2019-05-09 18:36 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Lemon @ 2019-05-09 16:12 UTC (permalink / raw)
To: Björn Töpel
Cc: Netdev, Björn Töpel, Karlsson, Magnus,
Alexei Starovoitov, kernel-team
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.
--
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap
2019-05-09 16:12 ` Jonathan Lemon
@ 2019-05-09 18:36 ` Alexei Starovoitov
2019-05-13 9:29 ` Björn Töpel
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2019-05-09 18:36 UTC (permalink / raw)
To: Jonathan Lemon, Björn Töpel
Cc: Netdev, Björn Töpel, Karlsson, Magnus,
Alexei Starovoitov, Kernel Team
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap
2019-05-09 18:36 ` Alexei Starovoitov
@ 2019-05-13 9:29 ` Björn Töpel
0 siblings, 0 replies; 6+ messages in thread
From: Björn Töpel @ 2019-05-13 9:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jonathan Lemon, Netdev, Björn Töpel, Karlsson, Magnus,
Alexei Starovoitov, Kernel Team
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-13 9:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 22:50 [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon
2019-05-08 22:50 ` [PATCH bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon
2019-05-09 11:48 ` [PATCH bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Björn Töpel
2019-05-09 16:12 ` Jonathan Lemon
2019-05-09 18:36 ` Alexei Starovoitov
2019-05-13 9:29 ` Björn Töpel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).