| Message ID | 1601645787-16944-1-git-send-email-magnus.karlsson@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [bpf-next] libbpf: fix compatibility problem in xsk_socket__create | expand |
On 10/2/20 3:36 PM, Magnus Karlsson wrote: > From: Magnus Karlsson <magnus.karlsson@intel.com> > > Fix a compatibility problem when the old XDP_SHARED_UMEM mode is used > together with the xsk_socket__create() call. In the old XDP_SHARED_UMEM > mode, only sharing of the same device and queue id was allowed, and in > this mode, the fill ring and completion ring were shared between the > AF_XDP sockets. Therfore, it was perfectly fine to call the > xsk_socket__create() API for each socket and not use the new > xsk_socket__create_shared() API. This behaviour was ruined by the > commit introducing XDP_SHARED_UMEM support between different devices > and/or queue ids. This patch restores the ability to use > xsk_socket__create in these circumstances so that backward > compatibility is not broken. > > We also make sure that a user that uses the > xsk_socket__create_shared() api for the first socket in the old > XDP_SHARED_UMEM mode above, gets and error message if the user tries > to feed a fill ring or a completion ring that is not the same as the > ones used for the umem registration. Previously, libbpf would just > have silently ignored the supplied fill and completion rings and just > taken them from the umem. Better to provide an error to the user. > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com> > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices") > --- > tools/lib/bpf/xsk.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 30b4ca5..5b61932 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -705,7 +705,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > struct xsk_ctx *ctx; > int err, ifindex; > > - if (!umem || !xsk_ptr || !(rx || tx) || !fill || !comp) > + if (!umem || !xsk_ptr || !(rx || tx)) > return -EFAULT; > > xsk = calloc(1, sizeof(*xsk)); > @@ -735,12 +735,24 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > > ctx = xsk_get_ctx(umem, ifindex, queue_id); > if (!ctx) { > + if (!fill || !comp) { > + err = -EFAULT; > + goto out_socket; > + } > + > ctx = xsk_create_ctx(xsk, umem, ifindex, ifname, queue_id, > fill, comp); > if (!ctx) { > err = -ENOMEM; > goto out_socket; > } > + } else if ((fill && ctx->fill != fill) || (comp && ctx->comp != comp)) { > + /* If the xsk_socket__create_shared() api is used for the first socket > + * registration, then make sure the fill and completion rings supplied > + * are the same as the ones used to register the umem. If not, bail out. > + */ > + err = -EINVAL; > + goto out_socket; This looks buggy. You got a valid ctx in this path which was ctx->refcount++'ed. By just going to out_socket you'll leak this libbpf internal refcount. > } > xsk->ctx = ctx; > >
On Mon, Oct 5, 2020 at 4:37 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 10/2/20 3:36 PM, Magnus Karlsson wrote: > > From: Magnus Karlsson <magnus.karlsson@intel.com> > > > > Fix a compatibility problem when the old XDP_SHARED_UMEM mode is used > > together with the xsk_socket__create() call. In the old XDP_SHARED_UMEM > > mode, only sharing of the same device and queue id was allowed, and in > > this mode, the fill ring and completion ring were shared between the > > AF_XDP sockets. Therfore, it was perfectly fine to call the > > xsk_socket__create() API for each socket and not use the new > > xsk_socket__create_shared() API. This behaviour was ruined by the > > commit introducing XDP_SHARED_UMEM support between different devices > > and/or queue ids. This patch restores the ability to use > > xsk_socket__create in these circumstances so that backward > > compatibility is not broken. > > > > We also make sure that a user that uses the > > xsk_socket__create_shared() api for the first socket in the old > > XDP_SHARED_UMEM mode above, gets and error message if the user tries > > to feed a fill ring or a completion ring that is not the same as the > > ones used for the umem registration. Previously, libbpf would just > > have silently ignored the supplied fill and completion rings and just > > taken them from the umem. Better to provide an error to the user. > > > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com> > > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices") > > --- > > tools/lib/bpf/xsk.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > index 30b4ca5..5b61932 100644 > > --- a/tools/lib/bpf/xsk.c > > +++ b/tools/lib/bpf/xsk.c > > @@ -705,7 +705,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > > struct xsk_ctx *ctx; > > int err, ifindex; > > > > - if (!umem || !xsk_ptr || !(rx || tx) || !fill || !comp) > > + if (!umem || !xsk_ptr || !(rx || tx)) > > return -EFAULT; > > > > xsk = calloc(1, sizeof(*xsk)); > > @@ -735,12 +735,24 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > > > > ctx = xsk_get_ctx(umem, ifindex, queue_id); > > if (!ctx) { > > + if (!fill || !comp) { > > + err = -EFAULT; > > + goto out_socket; > > + } > > + > > ctx = xsk_create_ctx(xsk, umem, ifindex, ifname, queue_id, > > fill, comp); > > if (!ctx) { > > err = -ENOMEM; > > goto out_socket; > > } > > + } else if ((fill && ctx->fill != fill) || (comp && ctx->comp != comp)) { > > + /* If the xsk_socket__create_shared() api is used for the first socket > > + * registration, then make sure the fill and completion rings supplied > > + * are the same as the ones used to register the umem. If not, bail out. > > + */ > > + err = -EINVAL; > > + goto out_socket; > > This looks buggy. You got a valid ctx in this path which was ctx->refcount++'ed. By just > going to out_socket you'll leak this libbpf internal refcount. Yes, you are correct. Thanks for spotting. It jumps to the wrong label. It should be: goto out_put_ctx; so that ctx refcount is decreased. Will submit a v2. > > } > > xsk->ctx = ctx; > > > > >
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 30b4ca5..5b61932 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -705,7 +705,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, struct xsk_ctx *ctx; int err, ifindex; - if (!umem || !xsk_ptr || !(rx || tx) || !fill || !comp) + if (!umem || !xsk_ptr || !(rx || tx)) return -EFAULT; xsk = calloc(1, sizeof(*xsk)); @@ -735,12 +735,24 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, ctx = xsk_get_ctx(umem, ifindex, queue_id); if (!ctx) { + if (!fill || !comp) { + err = -EFAULT; + goto out_socket; + } + ctx = xsk_create_ctx(xsk, umem, ifindex, ifname, queue_id, fill, comp); if (!ctx) { err = -ENOMEM; goto out_socket; } + } else if ((fill && ctx->fill != fill) || (comp && ctx->comp != comp)) { + /* If the xsk_socket__create_shared() api is used for the first socket + * registration, then make sure the fill and completion rings supplied + * are the same as the ones used to register the umem. If not, bail out. + */ + err = -EINVAL; + goto out_socket; } xsk->ctx = ctx;