BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes
@ 2021-03-31  6:12 Ciara Loftus
  2021-03-31  6:12 ` [PATCH v4 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Ciara Loftus @ 2021-03-31  6:12 UTC (permalink / raw)
  To: netdev, bpf, magnus.karlsson, bjorn, alexei.starovoitov; +Cc: 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
introducing a new fields to struct xsk_umem which represent the ring
setup status for the xsk which shares the fd with the umem.

v3->v4:
* Reduced nesting in xsk_put_ctx as suggested by Alexei.
* Use bools instead of a u8 and flags to represent the
  ring setup status as suggested by Björn.

v2->v3:
* Instead of ignoring the return values of the setsockopt calls, introduce
  a new flag to determine whether or not to call them based on the ring
  setup status as suggested by Alexei.

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

This series applies on commit 861de02e5f3f2a104eecc5af1d248cb7bf8c5f75

Ciara Loftus (3):
  libbpf: ensure umem pointer is non-NULL before dereferencing
  libbpf: restore umem state after socket create failure
  libbpf: only create rx and tx XDP rings when necessary

 tools/lib/bpf/xsk.c | 57 +++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 20 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing
  2021-03-31  6:12 [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
@ 2021-03-31  6:12 ` Ciara Loftus
  2021-03-31  6:12 ` [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Ciara Loftus @ 2021-03-31  6:12 UTC (permalink / raw)
  To: netdev, bpf, magnus.karlsson, bjorn, alexei.starovoitov; +Cc: 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure
  2021-03-31  6:12 [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
  2021-03-31  6:12 ` [PATCH v4 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
@ 2021-03-31  6:12 ` Ciara Loftus
  2021-04-07 18:02   ` Andrii Nakryiko
  2021-03-31  6:12 ` [PATCH v4 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
  2021-03-31  7:05 ` [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Björn Töpel
  3 siblings, 1 reply; 8+ messages in thread
From: Ciara Loftus @ 2021-03-31  6:12 UTC (permalink / raw)
  To: netdev, bpf, magnus.karlsson, bjorn, alexei.starovoitov; +Cc: 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 | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 443b0cfb45e8..5098d9e3b55a 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -743,26 +743,30 @@ 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 (--ctx->refcount)
+		return;
 
-		list_del(&ctx->list);
-		free(ctx);
-	}
+	if (!unmap)
+		goto out_free;
+
+	err = xsk_get_mmap_offsets(umem->fd, &off);
+	if (err)
+		goto out_free;
+
+	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));
+
+out_free:
+	list_del(&ctx->list);
+	free(ctx);
 }
 
 static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
@@ -797,8 +801,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 +856,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 +997,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 +1010,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 +1076,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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary
  2021-03-31  6:12 [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
  2021-03-31  6:12 ` [PATCH v4 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
  2021-03-31  6:12 ` [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
@ 2021-03-31  6:12 ` Ciara Loftus
  2021-03-31  7:05 ` [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Björn Töpel
  3 siblings, 0 replies; 8+ messages in thread
From: Ciara Loftus @ 2021-03-31  6:12 UTC (permalink / raw)
  To: netdev, bpf, magnus.karlsson, bjorn, alexei.starovoitov; +Cc: Ciara Loftus

Prior to this commit xsk_socket__create(_shared) always attempted to create
the rx and tx rings for the socket. However this causes an issue when the
socket being setup is that which shares the fd with the UMEM. If a
previous call to this function failed with this socket after the rings were
set up, a subsequent call would always fail because the rings are not torn
down after the first call and when we try to set them up again we encounter
an error because they already exist. Solve this by remembering whether the
rings were set up by introducing new bools to struct xsk_umem which
represent the ring setup status and using them to determine whether or
not to set up the rings.

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

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

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 5098d9e3b55a..d24b5cc720ec 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -59,6 +59,8 @@ struct xsk_umem {
 	int fd;
 	int refcount;
 	struct list_head ctx_list;
+	bool rx_ring_setup_done;
+	bool tx_ring_setup_done;
 };
 
 struct xsk_ctx {
@@ -857,6 +859,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 	struct xsk_ctx *ctx;
 	int err, ifindex;
 	bool unmap = umem->fill_save != fill;
+	bool rx_setup_done = false, tx_setup_done = false;
 
 	if (!umem || !xsk_ptr || !(rx || tx))
 		return -EFAULT;
@@ -884,6 +887,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 		}
 	} else {
 		xsk->fd = umem->fd;
+		rx_setup_done = umem->rx_ring_setup_done;
+		tx_setup_done = umem->tx_ring_setup_done;
 	}
 
 	ctx = xsk_get_ctx(umem, ifindex, queue_id);
@@ -902,7 +907,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 	}
 	xsk->ctx = ctx;
 
-	if (rx) {
+	if (rx && !rx_setup_done) {
 		err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,
 				 &xsk->config.rx_size,
 				 sizeof(xsk->config.rx_size));
@@ -910,8 +915,10 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 			err = -errno;
 			goto out_put_ctx;
 		}
+		if (xsk->fd == umem->fd)
+			umem->rx_ring_setup_done = true;
 	}
-	if (tx) {
+	if (tx && !tx_setup_done) {
 		err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING,
 				 &xsk->config.tx_size,
 				 sizeof(xsk->config.tx_size));
@@ -919,6 +926,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 			err = -errno;
 			goto out_put_ctx;
 		}
+		if (xsk->fd == umem->fd)
+			umem->rx_ring_setup_done = true;
 	}
 
 	err = xsk_get_mmap_offsets(xsk->fd, &off);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes
  2021-03-31  6:12 [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
                   ` (2 preceding siblings ...)
  2021-03-31  6:12 ` [PATCH v4 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
@ 2021-03-31  7:05 ` Björn Töpel
  2021-04-01 21:49   ` Alexei Starovoitov
  3 siblings, 1 reply; 8+ messages in thread
From: Björn Töpel @ 2021-03-31  7:05 UTC (permalink / raw)
  To: Ciara Loftus; +Cc: Netdev, bpf, Karlsson, Magnus, Alexei Starovoitov

On Wed, 31 Mar 2021 at 08:43, Ciara Loftus <ciara.loftus@intel.com> wrote:
>
> 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
> introducing a new fields to struct xsk_umem which represent the ring
> setup status for the xsk which shares the fd with the umem.
>
> v3->v4:
> * Reduced nesting in xsk_put_ctx as suggested by Alexei.
> * Use bools instead of a u8 and flags to represent the
>   ring setup status as suggested by Björn.
>

Thanks, Ciara! LGTM!

Acked-by: Björn Töpel <bjorn@kernel.org>


> v2->v3:
> * Instead of ignoring the return values of the setsockopt calls, introduce
>   a new flag to determine whether or not to call them based on the ring
>   setup status as suggested by Alexei.
>
> v1->v2:
> * Simplified restoring the _save pointers as suggested by Magnus.
> * Fixed the condition which determines whether to unmap umem rings
>  when socket create fails.
>
> This series applies on commit 861de02e5f3f2a104eecc5af1d248cb7bf8c5f75
>
> Ciara Loftus (3):
>   libbpf: ensure umem pointer is non-NULL before dereferencing
>   libbpf: restore umem state after socket create failure
>   libbpf: only create rx and tx XDP rings when necessary
>
>  tools/lib/bpf/xsk.c | 57 +++++++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
>
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes
  2021-03-31  7:05 ` [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Björn Töpel
@ 2021-04-01 21:49   ` Alexei Starovoitov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2021-04-01 21:49 UTC (permalink / raw)
  To: Björn Töpel; +Cc: Ciara Loftus, Netdev, bpf, Karlsson, Magnus

On Wed, Mar 31, 2021 at 12:06 AM Björn Töpel <bjorn@kernel.org> wrote:
>
> On Wed, 31 Mar 2021 at 08:43, Ciara Loftus <ciara.loftus@intel.com> wrote:
> >
> > 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
> > introducing a new fields to struct xsk_umem which represent the ring
> > setup status for the xsk which shares the fd with the umem.
> >
> > v3->v4:
> > * Reduced nesting in xsk_put_ctx as suggested by Alexei.
> > * Use bools instead of a u8 and flags to represent the
> >   ring setup status as suggested by Björn.
> >
>
> Thanks, Ciara! LGTM!
>
> Acked-by: Björn Töpel <bjorn@kernel.org>

Applied. Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure
  2021-03-31  6:12 ` [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
@ 2021-04-07 18:02   ` Andrii Nakryiko
  2021-04-08  5:52     ` Loftus, Ciara
  0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2021-04-07 18:02 UTC (permalink / raw)
  To: Ciara Loftus
  Cc: Networking, bpf, Magnus Karlsson, Björn Töpel,
	Alexei Starovoitov

On Tue, Mar 30, 2021 at 11:45 PM Ciara Loftus <ciara.loftus@intel.com> wrote:
>
> 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 | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 443b0cfb45e8..5098d9e3b55a 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -743,26 +743,30 @@ 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 (--ctx->refcount)
> +               return;
>
> -               list_del(&ctx->list);
> -               free(ctx);
> -       }
> +       if (!unmap)
> +               goto out_free;
> +
> +       err = xsk_get_mmap_offsets(umem->fd, &off);
> +       if (err)
> +               goto out_free;
> +
> +       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));
> +
> +out_free:
> +       list_del(&ctx->list);
> +       free(ctx);
>  }
>
>  static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
> @@ -797,8 +801,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 +856,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;
>

we are checking !umem only on the next line, so here it can be still
NULL. Please send a fix, thanks.

>         if (!umem || !xsk_ptr || !(rx || tx))
>                 return -EFAULT;
> @@ -994,6 +997,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 +1010,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 +1076,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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure
  2021-04-07 18:02   ` Andrii Nakryiko
@ 2021-04-08  5:52     ` Loftus, Ciara
  0 siblings, 0 replies; 8+ messages in thread
From: Loftus, Ciara @ 2021-04-08  5:52 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Networking, bpf, Karlsson, Magnus, Björn Töpel,
	Alexei Starovoitov

> On Tue, Mar 30, 2021 at 11:45 PM Ciara Loftus <ciara.loftus@intel.com>
> wrote:
> >
> > 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 | 41 +++++++++++++++++++++++------------------
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index 443b0cfb45e8..5098d9e3b55a 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -743,26 +743,30 @@ 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 (--ctx->refcount)
> > +               return;
> >
> > -               list_del(&ctx->list);
> > -               free(ctx);
> > -       }
> > +       if (!unmap)
> > +               goto out_free;
> > +
> > +       err = xsk_get_mmap_offsets(umem->fd, &off);
> > +       if (err)
> > +               goto out_free;
> > +
> > +       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));
> > +
> > +out_free:
> > +       list_del(&ctx->list);
> > +       free(ctx);
> >  }
> >
> >  static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
> > @@ -797,8 +801,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 +856,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;
> >
> 
> we are checking !umem only on the next line, so here it can be still
> NULL. Please send a fix, thanks.

Thank you for catching this. I've sent a fix.

Ciara

> 
> >         if (!umem || !xsk_ptr || !(rx || tx))
> >                 return -EFAULT;
> > @@ -994,6 +997,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 +1010,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 +1076,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
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-31  6:12 [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
2021-03-31  6:12 ` [PATCH v4 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
2021-03-31  6:12 ` [PATCH v4 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
2021-04-07 18:02   ` Andrii Nakryiko
2021-04-08  5:52     ` Loftus, Ciara
2021-03-31  6:12 ` [PATCH v4 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
2021-03-31  7:05 ` [PATCH v4 bpf 0/3] AF_XDP Socket Creation Fixes Björn Töpel
2021-04-01 21:49   ` Alexei Starovoitov

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git