* [PATCH v4 bpf-next 0/2] Better handling of xskmap entries @ 2019-06-03 16:38 Jonathan Lemon 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Jonathan Lemon @ 2019-06-03 16:38 UTC (permalink / raw) To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson, ast, daniel v3->v4: - Clarify error handling path. v2->v3: - Use correct map type. Jonathan Lemon (2): bpf: Allow bpf_map_lookup_elem() on an xskmap libbpf: remove qidconf and better support external bpf programs. include/net/xdp_sock.h | 6 +- kernel/bpf/verifier.c | 6 +- kernel/bpf/xskmap.c | 4 +- tools/lib/bpf/xsk.c | 103 +++++------------- .../bpf/verifier/prevent_map_lookup.c | 15 --- 5 files changed, 39 insertions(+), 95 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-03 16:38 [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon @ 2019-06-03 16:38 ` Jonathan Lemon 2019-06-04 14:54 ` Daniel Borkmann 2019-06-04 16:43 ` Jesper Dangaard Brouer 2019-06-03 16:38 ` [PATCH v4 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon 2019-06-04 6:06 ` [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Björn Töpel 2 siblings, 2 replies; 13+ messages in thread From: Jonathan Lemon @ 2019-06-03 16:38 UTC (permalink / raw) To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson, ast, daniel 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 the queue_id, as a way of indicating that there is a valid entry at the map index. Rearrange some xdp_sock members to eliminate structure holes. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> Acked-by: Song Liu <songliubraving@fb.com> Acked-by: Björn Töpel <bjorn.topel@intel.com> --- include/net/xdp_sock.h | 6 +++--- kernel/bpf/verifier.c | 6 +++++- kernel/bpf/xskmap.c | 4 +++- .../selftests/bpf/verifier/prevent_map_lookup.c | 15 --------------- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h index d074b6d60f8a..7d84b1da43d2 100644 --- a/include/net/xdp_sock.h +++ b/include/net/xdp_sock.h @@ -57,12 +57,12 @@ struct xdp_sock { struct net_device *dev; struct xdp_umem *umem; struct list_head flush_node; - u16 queue_id; - struct xsk_queue *tx ____cacheline_aligned_in_smp; - struct list_head list; + u32 queue_id; bool zc; /* Protects multiple processes in the control path */ struct mutex mutex; + struct xsk_queue *tx ____cacheline_aligned_in_smp; + struct list_head list; /* Mutual exclusion of NAPI TX thread and sendmsg error paths * in the SKB destructor callback. */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2778417e6e0c..91c730f85e92 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2905,10 +2905,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..249b22089014 100644 --- a/kernel/bpf/xskmap.c +++ b/kernel/bpf/xskmap.c @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map) static void *xsk_map_lookup_elem(struct bpf_map *map, void *key) { - return ERR_PTR(-EOPNOTSUPP); + struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key); + + return xs ? &xs->queue_id : NULL; } 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] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon @ 2019-06-04 14:54 ` Daniel Borkmann 2019-06-04 15:30 ` Daniel Borkmann 2019-06-04 16:43 ` Jesper Dangaard Brouer 1 sibling, 1 reply; 13+ messages in thread From: Daniel Borkmann @ 2019-06-04 14:54 UTC (permalink / raw) To: Jonathan Lemon, netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson, ast On 06/03/2019 06:38 PM, Jonathan Lemon 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 the queue_id, as a way of > indicating that there is a valid entry at the map index. > > Rearrange some xdp_sock members to eliminate structure holes. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > Acked-by: Song Liu <songliubraving@fb.com> > Acked-by: Björn Töpel <bjorn.topel@intel.com> > --- > include/net/xdp_sock.h | 6 +++--- > kernel/bpf/verifier.c | 6 +++++- > kernel/bpf/xskmap.c | 4 +++- > .../selftests/bpf/verifier/prevent_map_lookup.c | 15 --------------- > 4 files changed, 11 insertions(+), 20 deletions(-) > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h > index d074b6d60f8a..7d84b1da43d2 100644 > --- a/include/net/xdp_sock.h > +++ b/include/net/xdp_sock.h > @@ -57,12 +57,12 @@ struct xdp_sock { > struct net_device *dev; > struct xdp_umem *umem; > struct list_head flush_node; > - u16 queue_id; > - struct xsk_queue *tx ____cacheline_aligned_in_smp; > - struct list_head list; > + u32 queue_id; > bool zc; > /* Protects multiple processes in the control path */ > struct mutex mutex; > + struct xsk_queue *tx ____cacheline_aligned_in_smp; > + struct list_head list; > /* Mutual exclusion of NAPI TX thread and sendmsg error paths > * in the SKB destructor callback. > */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 2778417e6e0c..91c730f85e92 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -2905,10 +2905,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..249b22089014 100644 > --- a/kernel/bpf/xskmap.c > +++ b/kernel/bpf/xskmap.c > @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map) > > static void *xsk_map_lookup_elem(struct bpf_map *map, void *key) > { > - return ERR_PTR(-EOPNOTSUPP); > + struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key); > + > + return xs ? &xs->queue_id : NULL; > } How do you guarantee that BPf programs don't mess around with the map values e.g. overriding xs->queue_id from the lookup? This should be read-only map from BPF program side. > 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 = { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 14:54 ` Daniel Borkmann @ 2019-06-04 15:30 ` Daniel Borkmann 0 siblings, 0 replies; 13+ messages in thread From: Daniel Borkmann @ 2019-06-04 15:30 UTC (permalink / raw) To: Jonathan Lemon, netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson, ast On 06/04/2019 04:54 PM, Daniel Borkmann wrote: > On 06/03/2019 06:38 PM, Jonathan Lemon 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 the queue_id, as a way of >> indicating that there is a valid entry at the map index. >> >> Rearrange some xdp_sock members to eliminate structure holes. >> >> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> >> Acked-by: Song Liu <songliubraving@fb.com> >> Acked-by: Björn Töpel <bjorn.topel@intel.com> >> --- >> include/net/xdp_sock.h | 6 +++--- >> kernel/bpf/verifier.c | 6 +++++- >> kernel/bpf/xskmap.c | 4 +++- >> .../selftests/bpf/verifier/prevent_map_lookup.c | 15 --------------- >> 4 files changed, 11 insertions(+), 20 deletions(-) >> >> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h >> index d074b6d60f8a..7d84b1da43d2 100644 >> --- a/include/net/xdp_sock.h >> +++ b/include/net/xdp_sock.h >> @@ -57,12 +57,12 @@ struct xdp_sock { >> struct net_device *dev; >> struct xdp_umem *umem; >> struct list_head flush_node; >> - u16 queue_id; >> - struct xsk_queue *tx ____cacheline_aligned_in_smp; >> - struct list_head list; >> + u32 queue_id; >> bool zc; >> /* Protects multiple processes in the control path */ >> struct mutex mutex; >> + struct xsk_queue *tx ____cacheline_aligned_in_smp; >> + struct list_head list; >> /* Mutual exclusion of NAPI TX thread and sendmsg error paths >> * in the SKB destructor callback. >> */ >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 2778417e6e0c..91c730f85e92 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -2905,10 +2905,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..249b22089014 100644 >> --- a/kernel/bpf/xskmap.c >> +++ b/kernel/bpf/xskmap.c >> @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map) >> >> static void *xsk_map_lookup_elem(struct bpf_map *map, void *key) >> { >> - return ERR_PTR(-EOPNOTSUPP); >> + struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key); >> + >> + return xs ? &xs->queue_id : NULL; >> } > > How do you guarantee that BPf programs don't mess around with the map values > e.g. overriding xs->queue_id from the lookup? This should be read-only map > from BPF program side. (Or via per-cpu scratch var where you move xs->queue_id into and return from here.) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon 2019-06-04 14:54 ` Daniel Borkmann @ 2019-06-04 16:43 ` Jesper Dangaard Brouer 2019-06-04 17:25 ` Jonathan Lemon 1 sibling, 1 reply; 13+ messages in thread From: Jesper Dangaard Brouer @ 2019-06-04 16:43 UTC (permalink / raw) To: Jonathan Lemon Cc: brouer, netdev, kernel-team, bjorn.topel, magnus.karlsson, ast, daniel On Mon, 3 Jun 2019 09:38:51 -0700 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 the queue_id, as a way of > indicating that there is a valid entry at the map index. Just a reminder, that once we choose a return value, there the queue_id, then it basically becomes UAPI, and we cannot change it. Can we somehow use BTF to allow us to extend this later? I was also going to point out that, you cannot return a direct pointer to queue_id, as BPF-prog side can modify this... but Daniel already pointed this out. -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 16:43 ` Jesper Dangaard Brouer @ 2019-06-04 17:25 ` Jonathan Lemon 2019-06-04 18:12 ` Martin Lau ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Jonathan Lemon @ 2019-06-04 17:25 UTC (permalink / raw) To: Jesper Dangaard Brouer Cc: netdev, kernel-team, bjorn.topel, magnus.karlsson, ast, daniel On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: > On Mon, 3 Jun 2019 09:38:51 -0700 > 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 the queue_id, as a way of >> indicating that there is a valid entry at the map index. > > Just a reminder, that once we choose a return value, there the > queue_id, then it basically becomes UAPI, and we cannot change it. Yes - Alexei initially wanted to return the sk_cookie instead, but that's 64 bits and opens up a whole other can of worms. > Can we somehow use BTF to allow us to extend this later? > > I was also going to point out that, you cannot return a direct pointer > to queue_id, as BPF-prog side can modify this... but Daniel already > pointed this out. So, I see three solutions here (for this and Toke's patchset also, which is encountering the same problem). 1) add a scratch register (Toke's approach) 2) add a PTR_TO_<type>, which has the access checked. This is the most flexible approach, but does seem a bit overkill at the moment. 3) add another helper function, say, bpf_map_elem_present() which just returns a boolean value indicating whether there is a valid map entry or not. I was starting to do 2), but wanted to get some more feedback first. -- Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 17:25 ` Jonathan Lemon @ 2019-06-04 18:12 ` Martin Lau 2019-06-05 8:45 ` Björn Töpel 2019-06-04 18:18 ` Björn Töpel 2019-06-04 20:07 ` Toke Høiland-Jørgensen 2 siblings, 1 reply; 13+ messages in thread From: Martin Lau @ 2019-06-04 18:12 UTC (permalink / raw) To: Jonathan Lemon Cc: Jesper Dangaard Brouer, netdev, Kernel Team, bjorn.topel, magnus.karlsson, ast, daniel On Tue, Jun 04, 2019 at 10:25:23AM -0700, Jonathan Lemon wrote: > On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: > > > On Mon, 3 Jun 2019 09:38:51 -0700 > > 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 the queue_id, as a way of > >> indicating that there is a valid entry at the map index. > > > > Just a reminder, that once we choose a return value, there the > > queue_id, then it basically becomes UAPI, and we cannot change it. > > Yes - Alexei initially wanted to return the sk_cookie instead, but > that's 64 bits and opens up a whole other can of worms. > > > > Can we somehow use BTF to allow us to extend this later? > > > > I was also going to point out that, you cannot return a direct pointer > > to queue_id, as BPF-prog side can modify this... but Daniel already > > pointed this out. > > So, I see three solutions here (for this and Toke's patchset also, > which is encountering the same problem). > > 1) add a scratch register (Toke's approach) > 2) add a PTR_TO_<type>, which has the access checked. This is the most > flexible approach, but does seem a bit overkill at the moment. I think it would be nice and more extensible to have PTR_TO_xxx. It could start with the existing PTR_TO_SOCKET or starting with a new PTR_TO_XDP_SOCK from the beginning is also fine. > 3) add another helper function, say, bpf_map_elem_present() which just > returns a boolean value indicating whether there is a valid map entry > or not. > > I was starting to do 2), but wanted to get some more feedback first. > -- > Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 18:12 ` Martin Lau @ 2019-06-05 8:45 ` Björn Töpel 2019-06-05 15:42 ` Jonathan Lemon 0 siblings, 1 reply; 13+ messages in thread From: Björn Töpel @ 2019-06-05 8:45 UTC (permalink / raw) To: Martin Lau Cc: Jonathan Lemon, Jesper Dangaard Brouer, netdev, Kernel Team, bjorn.topel, magnus.karlsson, ast, daniel On Tue, 4 Jun 2019 at 20:13, Martin Lau <kafai@fb.com> wrote: > > On Tue, Jun 04, 2019 at 10:25:23AM -0700, Jonathan Lemon wrote: > > On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: > > > > > On Mon, 3 Jun 2019 09:38:51 -0700 > > > 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 the queue_id, as a way of > > >> indicating that there is a valid entry at the map index. > > > > > > Just a reminder, that once we choose a return value, there the > > > queue_id, then it basically becomes UAPI, and we cannot change it. > > > > Yes - Alexei initially wanted to return the sk_cookie instead, but > > that's 64 bits and opens up a whole other can of worms. > > > > > > > Can we somehow use BTF to allow us to extend this later? > > > > > > I was also going to point out that, you cannot return a direct pointer > > > to queue_id, as BPF-prog side can modify this... but Daniel already > > > pointed this out. > > > > So, I see three solutions here (for this and Toke's patchset also, > > which is encountering the same problem). > > > > 1) add a scratch register (Toke's approach) > > 2) add a PTR_TO_<type>, which has the access checked. This is the most > > flexible approach, but does seem a bit overkill at the moment. > I think it would be nice and more extensible to have PTR_TO_xxx. > It could start with the existing PTR_TO_SOCKET > > or starting with a new PTR_TO_XDP_SOCK from the beginning is also fine. > Doesn't the PTR_TO_SOCKET path involve taking a ref and mandating sk_release() from the fast path? :-( Björn > > 3) add another helper function, say, bpf_map_elem_present() which just > > returns a boolean value indicating whether there is a valid map entry > > or not. > > > > I was starting to do 2), but wanted to get some more feedback first. > > -- > > Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-05 8:45 ` Björn Töpel @ 2019-06-05 15:42 ` Jonathan Lemon 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Lemon @ 2019-06-05 15:42 UTC (permalink / raw) To: Björn Töpel Cc: Martin Lau, Jesper Dangaard Brouer, netdev, Kernel Team, bjorn.topel, magnus.karlsson, ast, daniel On 5 Jun 2019, at 1:45, Björn Töpel wrote: > On Tue, 4 Jun 2019 at 20:13, Martin Lau <kafai@fb.com> wrote: >> >> On Tue, Jun 04, 2019 at 10:25:23AM -0700, Jonathan Lemon wrote: >>> On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: >>> >>>> On Mon, 3 Jun 2019 09:38:51 -0700 >>>> 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 the queue_id, as a way of >>>>> indicating that there is a valid entry at the map index. >>>> >>>> Just a reminder, that once we choose a return value, there the >>>> queue_id, then it basically becomes UAPI, and we cannot change it. >>> >>> Yes - Alexei initially wanted to return the sk_cookie instead, but >>> that's 64 bits and opens up a whole other can of worms. >>> >>> >>>> Can we somehow use BTF to allow us to extend this later? >>>> >>>> I was also going to point out that, you cannot return a direct pointer >>>> to queue_id, as BPF-prog side can modify this... but Daniel already >>>> pointed this out. >>> >>> So, I see three solutions here (for this and Toke's patchset also, >>> which is encountering the same problem). >>> >>> 1) add a scratch register (Toke's approach) >>> 2) add a PTR_TO_<type>, which has the access checked. This is the most >>> flexible approach, but does seem a bit overkill at the moment. >> I think it would be nice and more extensible to have PTR_TO_xxx. >> It could start with the existing PTR_TO_SOCKET >> >> or starting with a new PTR_TO_XDP_SOCK from the beginning is also fine. >> > > Doesn't the PTR_TO_SOCKET path involve taking a ref and mandating > sk_release() from the fast path? :-( AF_XDP sockets are created with SOCK_RCU_FREE, and used under rcu, so I don't think that they need to be refcounted. bpf_sk_release is a NOP in the RCU_FREE case. -- Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 17:25 ` Jonathan Lemon 2019-06-04 18:12 ` Martin Lau @ 2019-06-04 18:18 ` Björn Töpel 2019-06-04 20:07 ` Toke Høiland-Jørgensen 2 siblings, 0 replies; 13+ messages in thread From: Björn Töpel @ 2019-06-04 18:18 UTC (permalink / raw) To: Jonathan Lemon, Jesper Dangaard Brouer Cc: netdev, kernel-team, magnus.karlsson, ast, daniel On 2019-06-04 19:25, Jonathan Lemon wrote: > On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: > >> On Mon, 3 Jun 2019 09:38:51 -0700 >> 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 the queue_id, as a way of >>> indicating that there is a valid entry at the map index. >> >> Just a reminder, that once we choose a return value, there the >> queue_id, then it basically becomes UAPI, and we cannot change it. > > Yes - Alexei initially wanted to return the sk_cookie instead, but > that's 64 bits and opens up a whole other can of worms. > Hmm, what other info would be useful? ifindex? Or going the the other way, with read-only and just returning boolean? > >> Can we somehow use BTF to allow us to extend this later? >> >> I was also going to point out that, you cannot return a direct pointer >> to queue_id, as BPF-prog side can modify this... but Daniel already >> pointed this out. > Ugh, good thing Daniel found this! Björn > So, I see three solutions here (for this and Toke's patchset also, > which is encountering the same problem). > > 1) add a scratch register (Toke's approach) > 2) add a PTR_TO_<type>, which has the access checked. This is the most > flexible approach, but does seem a bit overkill at the moment. > 3) add another helper function, say, bpf_map_elem_present() which just > returns a boolean value indicating whether there is a valid map entry > or not. > > I was starting to do 2), but wanted to get some more feedback first. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-06-04 17:25 ` Jonathan Lemon 2019-06-04 18:12 ` Martin Lau 2019-06-04 18:18 ` Björn Töpel @ 2019-06-04 20:07 ` Toke Høiland-Jørgensen 2 siblings, 0 replies; 13+ messages in thread From: Toke Høiland-Jørgensen @ 2019-06-04 20:07 UTC (permalink / raw) To: Jonathan Lemon, Jesper Dangaard Brouer Cc: netdev, kernel-team, bjorn.topel, magnus.karlsson, ast, daniel Jonathan Lemon <jonathan.lemon@gmail.com> writes: > On 4 Jun 2019, at 9:43, Jesper Dangaard Brouer wrote: > >> On Mon, 3 Jun 2019 09:38:51 -0700 >> 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 the queue_id, as a way of >>> indicating that there is a valid entry at the map index. >> >> Just a reminder, that once we choose a return value, there the >> queue_id, then it basically becomes UAPI, and we cannot change it. > > Yes - Alexei initially wanted to return the sk_cookie instead, but > that's 64 bits and opens up a whole other can of worms. > > >> Can we somehow use BTF to allow us to extend this later? >> >> I was also going to point out that, you cannot return a direct pointer >> to queue_id, as BPF-prog side can modify this... but Daniel already >> pointed this out. > > So, I see three solutions here (for this and Toke's patchset also, > which is encountering the same problem). > > 1) add a scratch register (Toke's approach) > 2) add a PTR_TO_<type>, which has the access checked. This is the most > flexible approach, but does seem a bit overkill at the moment. > 3) add another helper function, say, bpf_map_elem_present() which just > returns a boolean value indicating whether there is a valid map entry > or not. > > I was starting to do 2), but wanted to get some more feedback first. I think I prefer 2) over 3); since we have a verifier that can actually enforce something like read-only behaviour, actually having access to the value will probably be useful to someone. I can obviously live with 1) as well, of course (since I already did that; though I just now realise that I forgot to make the scratch space per-CPU)... :) -Toke ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-06-03 16:38 [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon @ 2019-06-03 16:38 ` Jonathan Lemon 2019-06-04 6:06 ` [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Björn Töpel 2 siblings, 0 replies; 13+ messages in thread From: Jonathan Lemon @ 2019-06-03 16:38 UTC (permalink / raw) To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson, ast, daniel 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. Clean up error handling path. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> Acked-by: Song Liu <songliubraving@fb.com> Tested-by: Björn Töpel <bjorn.topel@intel.com> --- tools/lib/bpf/xsk.c | 103 ++++++++++++-------------------------------- 1 file changed, 28 insertions(+), 75 deletions(-) diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 38667b62f1fe..7ef6293b4fd7 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_XSKMAP, "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) @@ -497,26 +449,27 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk) return err; err = xsk_load_xdp_prog(xsk); - if (err) - goto out_maps; + if (err) { + xsk_delete_bpf_maps(xsk); + return err; + } } else { xsk->prog_fd = bpf_prog_get_fd_by_id(prog_id); err = xsk_lookup_bpf_maps(xsk); - if (err) - goto out_load; + if (err) { + close(xsk->prog_fd); + return err; + } } err = xsk_set_bpf_maps(xsk); - if (err) - goto out_load; + if (err) { + xsk_delete_bpf_maps(xsk); + close(xsk->prog_fd); + return err; + } return 0; - -out_load: - close(xsk->prog_fd); -out_maps: - xsk_delete_bpf_maps(xsk); - return err; } int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname, @@ -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] 13+ messages in thread
* Re: [PATCH v4 bpf-next 0/2] Better handling of xskmap entries 2019-06-03 16:38 [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon 2019-06-03 16:38 ` [PATCH v4 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon @ 2019-06-04 6:06 ` Björn Töpel 2 siblings, 0 replies; 13+ messages in thread From: Björn Töpel @ 2019-06-04 6:06 UTC (permalink / raw) To: Jonathan Lemon Cc: Netdev, Kernel Team, Björn Töpel, Karlsson, Magnus, Alexei Starovoitov, Daniel Borkmann On Mon, 3 Jun 2019 at 19:49, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > v3->v4: > - Clarify error handling path. > > v2->v3: > - Use correct map type. > > Jonathan Lemon (2): > bpf: Allow bpf_map_lookup_elem() on an xskmap > libbpf: remove qidconf and better support external bpf programs. > Nice work! For the series, Acked-by: Björn Töpel <bjorn.topel@intel.com> > include/net/xdp_sock.h | 6 +- > kernel/bpf/verifier.c | 6 +- > kernel/bpf/xskmap.c | 4 +- > tools/lib/bpf/xsk.c | 103 +++++------------- > .../bpf/verifier/prevent_map_lookup.c | 15 --- > 5 files changed, 39 insertions(+), 95 deletions(-) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-06-05 15:42 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-03 16:38 [PATCH v4 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon 2019-06-03 16:38 ` [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon 2019-06-04 14:54 ` Daniel Borkmann 2019-06-04 15:30 ` Daniel Borkmann 2019-06-04 16:43 ` Jesper Dangaard Brouer 2019-06-04 17:25 ` Jonathan Lemon 2019-06-04 18:12 ` Martin Lau 2019-06-05 8:45 ` Björn Töpel 2019-06-05 15:42 ` Jonathan Lemon 2019-06-04 18:18 ` Björn Töpel 2019-06-04 20:07 ` Toke Høiland-Jørgensen 2019-06-03 16:38 ` [PATCH v4 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon 2019-06-04 6:06 ` [PATCH v4 bpf-next 0/2] Better handling of xskmap entries 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).