[PATCH v2 bpf 0/3] AF_XDP Socket Creation Fixes

[PATCH v2 bpf 0/3] AF_XDP Socket Creation Fixes

[Ciara Loftus]

This series fixes some issues around socket creation for AF_XDP.

Patch 1 fixes a potential NULL pointer dereference in
xsk_socket__create_shared.

Patch 2 ensures that the umem passed to xsk_socket__create(_shared)
remains unchanged in event of failure.

Patch 3 makes it possible for xsk_socket__create(_shared) to
succeed even if the rx and tx XDP rings have already been set up, by
ignoring the return value of the XDP_RX_RING/XDP_TX_RING setsockopt.
This removes a limitation which existed whereby a user could not retry
socket creation after a previous failed attempt.

It was chosen to solve the problem by ignoring the return values in
libbpf instead of modifying the setsockopt handling code in the kernel
in order to make it possible for the solution to be available across
all kernels, provided a new enough libbpf is available.

v1->v2:
* Simplified restoring the _save pointers as suggested by Magnus
  Karlsson.
* Fixed the condition which determines whether to unmap umem rings
  when socket create fails.

This series applies on commit 6032ebb54c60cae24329f6aba3ce0c1ca8ad6abe


Ciara Loftus (3):
  libbpf: ensure umem pointer is non-NULL before dereferencing
  libbpf: restore umem state after socket create failure
  libbpf: ignore return values of setsockopt for XDP rings.

 tools/lib/bpf/xsk.c | 66 ++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH v2 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing

[Ciara Loftus]

Calls to xsk_socket__create dereference the umem to access the
fill_save and comp_save pointers. Make sure the umem is non-NULL
before doing this.

Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 tools/lib/bpf/xsk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 526fc35c0b23..443b0cfb45e8 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -1019,6 +1019,9 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		       struct xsk_ring_cons *rx, struct xsk_ring_prod *tx,
 		       const struct xsk_socket_config *usr_config)
 {
+	if (!umem)
+		return -EFAULT;
+
 	return xsk_socket__create_shared(xsk_ptr, ifname, queue_id, umem,
 					 rx, tx, umem->fill_save,
 					 umem->comp_save, usr_config);
-- 
2.17.1

[PATCH v2 bpf 2/3] libbpf: restore umem state after socket create failure

[Ciara Loftus]

If the call to xsk_socket__create fails, the user may want to retry the
socket creation using the same umem. Ensure that the umem is in the
same state on exit if the call fails by:
1. ensuring the umem _save pointers are unmodified.
2. not unmapping the set of umem rings that were set up with the umem
during xsk_umem__create, since those maps existed before the call to
xsk_socket__create and should remain in tact even in the event of
failure.

Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 tools/lib/bpf/xsk.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 443b0cfb45e8..d4991ddff05a 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -743,21 +743,23 @@ static struct xsk_ctx *xsk_get_ctx(struct xsk_umem *umem, int ifindex,
 	return NULL;
 }
 
-static void xsk_put_ctx(struct xsk_ctx *ctx)
+static void xsk_put_ctx(struct xsk_ctx *ctx, bool unmap)
 {
 	struct xsk_umem *umem = ctx->umem;
 	struct xdp_mmap_offsets off;
 	int err;
 
 	if (--ctx->refcount == 0) {
-		err = xsk_get_mmap_offsets(umem->fd, &off);
-		if (!err) {
-			munmap(ctx->fill->ring - off.fr.desc,
-			       off.fr.desc + umem->config.fill_size *
-			       sizeof(__u64));
-			munmap(ctx->comp->ring - off.cr.desc,
-			       off.cr.desc + umem->config.comp_size *
-			       sizeof(__u64));
+		if (unmap) {
+			err = xsk_get_mmap_offsets(umem->fd, &off);
+			if (!err) {
+				munmap(ctx->fill->ring - off.fr.desc,
+				       off.fr.desc + umem->config.fill_size *
+				sizeof(__u64));
+				munmap(ctx->comp->ring - off.cr.desc,
+				       off.cr.desc + umem->config.comp_size *
+				sizeof(__u64));
+			}
 		}
 
 		list_del(&ctx->list);
@@ -797,8 +799,6 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
 	memcpy(ctx->ifname, ifname, IFNAMSIZ - 1);
 	ctx->ifname[IFNAMSIZ - 1] = '\0';
 
-	umem->fill_save = NULL;
-	umem->comp_save = NULL;
 	ctx->fill = fill;
 	ctx->comp = comp;
 	list_add(&ctx->list, &umem->ctx_list);
@@ -854,6 +854,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 	struct xsk_socket *xsk;
 	struct xsk_ctx *ctx;
 	int err, ifindex;
+	bool unmap = umem->fill_save != fill;
 
 	if (!umem || !xsk_ptr || !(rx || tx))
 		return -EFAULT;
@@ -994,6 +995,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 	}
 
 	*xsk_ptr = xsk;
+	umem->fill_save = NULL;
+	umem->comp_save = NULL;
 	return 0;
 
 out_mmap_tx:
@@ -1005,7 +1008,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 		munmap(rx_map, off.rx.desc +
 		       xsk->config.rx_size * sizeof(struct xdp_desc));
 out_put_ctx:
-	xsk_put_ctx(ctx);
+	xsk_put_ctx(ctx, unmap);
 out_socket:
 	if (--umem->refcount)
 		close(xsk->fd);
@@ -1071,7 +1074,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 		}
 	}
 
-	xsk_put_ctx(ctx);
+	xsk_put_ctx(ctx, true);
 
 	umem->refcount--;
 	/* Do not close an fd that also has an associated umem connected
-- 
2.17.1

[PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.

[Ciara Loftus]

During xsk_socket__create the XDP_RX_RING and XDP_TX_RING setsockopts
are called to create the rx and tx rings for the AF_XDP socket. If the ring
has already been set up, the setsockopt will return an error. However,
in the event of a failure during xsk_socket__create(_shared) after the
rings have been set up, the user may wish to retry the socket creation
using these pre-existing rings. In this case we can ignore the error
returned by the setsockopts. If there is a true error, the subsequent
call to mmap() will catch it.

Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index d4991ddff05a..cfc4abf505c3 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 	}
 	xsk->ctx = ctx;
 
-	if (rx) {
-		err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
-				 &xsk->config.rx_size,
-				 sizeof(xsk->config.rx_size));
-		if (err) {
-			err = -errno;
-			goto out_put_ctx;
-		}
-	}
-	if (tx) {
-		err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
-				 &xsk->config.tx_size,
-				 sizeof(xsk->config.tx_size));
-		if (err) {
-			err = -errno;
-			goto out_put_ctx;
-		}
-	}
+	/* The return values of these setsockopt calls are intentionally not checked.
+	 * If the ring has already been set up setsockopt will return an error. However,
+	 * this scenario is acceptable as the user may be retrying the socket creation
+	 * with rings which were set up in a previous but ultimately unsuccessful call
+	 * to xsk_socket__create(_shared). The call later to mmap() will fail if there
+	 * is a real issue and we handle that return value appropriately there.
+	 */
+	if (rx)
+		setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
+			   &xsk->config.rx_size,
+			   sizeof(xsk->config.rx_size));
+
+	if (tx)
+		setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
+			   &xsk->config.tx_size,
+			   sizeof(xsk->config.tx_size));
 
 	err = xsk_get_mmap_offsets(xsk->fd, &off);
 	if (err) {
-- 
2.17.1

Re: [PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.

[Alexei Starovoitov]

On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote:
> During xsk_socket__create the XDP_RX_RING and XDP_TX_RING setsockopts
> are called to create the rx and tx rings for the AF_XDP socket. If the ring
> has already been set up, the setsockopt will return an error. However,
> in the event of a failure during xsk_socket__create(_shared) after the
> rings have been set up, the user may wish to retry the socket creation
> using these pre-existing rings. In this case we can ignore the error
> returned by the setsockopts. If there is a true error, the subsequent
> call to mmap() will catch it.
> 
> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> 
> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>  tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index d4991ddff05a..cfc4abf505c3 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
>  	}
>  	xsk->ctx = ctx;
>  
> -	if (rx) {
> -		err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> -				 &xsk->config.rx_size,
> -				 sizeof(xsk->config.rx_size));
> -		if (err) {
> -			err = -errno;
> -			goto out_put_ctx;
> -		}
> -	}
> -	if (tx) {
> -		err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> -				 &xsk->config.tx_size,
> -				 sizeof(xsk->config.tx_size));
> -		if (err) {
> -			err = -errno;
> -			goto out_put_ctx;
> -		}
> -	}
> +	/* The return values of these setsockopt calls are intentionally not checked.
> +	 * If the ring has already been set up setsockopt will return an error. However,
> +	 * this scenario is acceptable as the user may be retrying the socket creation
> +	 * with rings which were set up in a previous but ultimately unsuccessful call
> +	 * to xsk_socket__create(_shared). The call later to mmap() will fail if there
> +	 * is a real issue and we handle that return value appropriately there.
> +	 */
> +	if (rx)
> +		setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> +			   &xsk->config.rx_size,
> +			   sizeof(xsk->config.rx_size));
> +
> +	if (tx)
> +		setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> +			   &xsk->config.tx_size,
> +			   sizeof(xsk->config.tx_size));

Instead of ignoring the error can you remember that setsockopt was done
in struct xsk_socket and don't do it the second time?

RE: [PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.

[Loftus, Ciara]

> 
> On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote:
> > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING
> setsockopts
> > are called to create the rx and tx rings for the AF_XDP socket. If the ring
> > has already been set up, the setsockopt will return an error. However,
> > in the event of a failure during xsk_socket__create(_shared) after the
> > rings have been set up, the user may wish to retry the socket creation
> > using these pre-existing rings. In this case we can ignore the error
> > returned by the setsockopts. If there is a true error, the subsequent
> > call to mmap() will catch it.
> >
> > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> >
> > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> >  tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------
> >  1 file changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index d4991ddff05a..cfc4abf505c3 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket
> **xsk_ptr,
> >  	}
> >  	xsk->ctx = ctx;
> >
> > -	if (rx) {
> > -		err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > -				 &xsk->config.rx_size,
> > -				 sizeof(xsk->config.rx_size));
> > -		if (err) {
> > -			err = -errno;
> > -			goto out_put_ctx;
> > -		}
> > -	}
> > -	if (tx) {
> > -		err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > -				 &xsk->config.tx_size,
> > -				 sizeof(xsk->config.tx_size));
> > -		if (err) {
> > -			err = -errno;
> > -			goto out_put_ctx;
> > -		}
> > -	}
> > +	/* The return values of these setsockopt calls are intentionally not
> checked.
> > +	 * If the ring has already been set up setsockopt will return an error.
> However,
> > +	 * this scenario is acceptable as the user may be retrying the socket
> creation
> > +	 * with rings which were set up in a previous but ultimately
> unsuccessful call
> > +	 * to xsk_socket__create(_shared). The call later to mmap() will fail if
> there
> > +	 * is a real issue and we handle that return value appropriately there.
> > +	 */
> > +	if (rx)
> > +		setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > +			   &xsk->config.rx_size,
> > +			   sizeof(xsk->config.rx_size));
> > +
> > +	if (tx)
> > +		setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > +			   &xsk->config.tx_size,
> > +			   sizeof(xsk->config.tx_size));
> 
> Instead of ignoring the error can you remember that setsockopt was done
> in struct xsk_socket and don't do it the second time?

Ideally we don't have to ignore the error. However in the event of failure struct xsk_socket is freed at the end of xsk_socket__create so we can't use it to remember state between subsequent calls to __create(). 

Re: [PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.

[Alexei Starovoitov]

On Mon, Mar 29, 2021 at 1:41 AM Loftus, Ciara <ciara.loftus@intel.com> wrote:
>
> >
> > On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote:
> > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING
> > setsockopts
> > > are called to create the rx and tx rings for the AF_XDP socket. If the ring
> > > has already been set up, the setsockopt will return an error. However,
> > > in the event of a failure during xsk_socket__create(_shared) after the
> > > rings have been set up, the user may wish to retry the socket creation
> > > using these pre-existing rings. In this case we can ignore the error
> > > returned by the setsockopts. If there is a true error, the subsequent
> > > call to mmap() will catch it.
> > >
> > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> > >
> > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > > ---
> > >  tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------
> > >  1 file changed, 16 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > index d4991ddff05a..cfc4abf505c3 100644
> > > --- a/tools/lib/bpf/xsk.c
> > > +++ b/tools/lib/bpf/xsk.c
> > > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket
> > **xsk_ptr,
> > >     }
> > >     xsk->ctx = ctx;
> > >
> > > -   if (rx) {
> > > -           err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > > -                            &xsk->config.rx_size,
> > > -                            sizeof(xsk->config.rx_size));
> > > -           if (err) {
> > > -                   err = -errno;
> > > -                   goto out_put_ctx;
> > > -           }
> > > -   }
> > > -   if (tx) {
> > > -           err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > > -                            &xsk->config.tx_size,
> > > -                            sizeof(xsk->config.tx_size));
> > > -           if (err) {
> > > -                   err = -errno;
> > > -                   goto out_put_ctx;
> > > -           }
> > > -   }
> > > +   /* The return values of these setsockopt calls are intentionally not
> > checked.
> > > +    * If the ring has already been set up setsockopt will return an error.
> > However,
> > > +    * this scenario is acceptable as the user may be retrying the socket
> > creation
> > > +    * with rings which were set up in a previous but ultimately
> > unsuccessful call
> > > +    * to xsk_socket__create(_shared). The call later to mmap() will fail if
> > there
> > > +    * is a real issue and we handle that return value appropriately there.
> > > +    */
> > > +   if (rx)
> > > +           setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > > +                      &xsk->config.rx_size,
> > > +                      sizeof(xsk->config.rx_size));
> > > +
> > > +   if (tx)
> > > +           setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > > +                      &xsk->config.tx_size,
> > > +                      sizeof(xsk->config.tx_size));
> >
> > Instead of ignoring the error can you remember that setsockopt was done
> > in struct xsk_socket and don't do it the second time?
>
> Ideally we don't have to ignore the error. However in the event of failure struct xsk_socket is freed at the end of xsk_socket__create so we can't use it to remember state between subsequent calls to __create().

but umem is not, right? and fd is taken from there.

RE: [PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.

[Loftus, Ciara]

> >
> > >
> > > On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote:
> > > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING
> > > setsockopts
> > > > are called to create the rx and tx rings for the AF_XDP socket. If the ring
> > > > has already been set up, the setsockopt will return an error. However,
> > > > in the event of a failure during xsk_socket__create(_shared) after the
> > > > rings have been set up, the user may wish to retry the socket creation
> > > > using these pre-existing rings. In this case we can ignore the error
> > > > returned by the setsockopts. If there is a true error, the subsequent
> > > > call to mmap() will catch it.
> > > >
> > > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
> > > >
> > > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > > > ---
> > > >  tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------
> > > >  1 file changed, 16 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > > index d4991ddff05a..cfc4abf505c3 100644
> > > > --- a/tools/lib/bpf/xsk.c
> > > > +++ b/tools/lib/bpf/xsk.c
> > > > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct
> xsk_socket
> > > **xsk_ptr,
> > > >     }
> > > >     xsk->ctx = ctx;
> > > >
> > > > -   if (rx) {
> > > > -           err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > > > -                            &xsk->config.rx_size,
> > > > -                            sizeof(xsk->config.rx_size));
> > > > -           if (err) {
> > > > -                   err = -errno;
> > > > -                   goto out_put_ctx;
> > > > -           }
> > > > -   }
> > > > -   if (tx) {
> > > > -           err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > > > -                            &xsk->config.tx_size,
> > > > -                            sizeof(xsk->config.tx_size));
> > > > -           if (err) {
> > > > -                   err = -errno;
> > > > -                   goto out_put_ctx;
> > > > -           }
> > > > -   }
> > > > +   /* The return values of these setsockopt calls are intentionally not
> > > checked.
> > > > +    * If the ring has already been set up setsockopt will return an error.
> > > However,
> > > > +    * this scenario is acceptable as the user may be retrying the socket
> > > creation
> > > > +    * with rings which were set up in a previous but ultimately
> > > unsuccessful call
> > > > +    * to xsk_socket__create(_shared). The call later to mmap() will fail if
> > > there
> > > > +    * is a real issue and we handle that return value appropriately there.
> > > > +    */
> > > > +   if (rx)
> > > > +           setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
> > > > +                      &xsk->config.rx_size,
> > > > +                      sizeof(xsk->config.rx_size));
> > > > +
> > > > +   if (tx)
> > > > +           setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
> > > > +                      &xsk->config.tx_size,
> > > > +                      sizeof(xsk->config.tx_size));
> > >
> > > Instead of ignoring the error can you remember that setsockopt was
> done
> > > in struct xsk_socket and don't do it the second time?
> >
> > Ideally we don't have to ignore the error. However in the event of failure
> struct xsk_socket is freed at the end of xsk_socket__create so we can't use it
> to remember state between subsequent calls to __create().
> 
> but umem is not, right? and fd is taken from there.

Yes, got it. We can add a new field to struct xsk_umem. It's much better than ignoring the return values.
I'll add this in the v3. Thanks for your suggestion!

Ciara