[PATCH bpf-next] libbpf: fix compatibility problem in xsk_socket__create

[PATCH bpf-next] libbpf: fix compatibility problem in xsk_socket__create

[Magnus Karlsson]

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;
 	}
 	xsk->ctx = ctx;
 
-- 
2.7.4

Re: [PATCH bpf-next] libbpf: fix compatibility problem in xsk_socket__create

[Daniel Borkmann]

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;
>   
> 

Re: [PATCH bpf-next] libbpf: fix compatibility problem in xsk_socket__create

[Magnus Karlsson]

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;
> >
> >
>