* [PATCH v3 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing
2021-03-30 11:34 [PATCH v3 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
@ 2021-03-30 11:34 ` Ciara Loftus
2021-03-30 11:34 ` [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
2021-03-30 11:34 ` [PATCH v3 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
2 siblings, 0 replies; 7+ messages in thread
From: Ciara Loftus @ 2021-03-30 11:34 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 related [flat|nested] 7+ messages in thread* [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure
2021-03-30 11:34 [PATCH v3 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
2021-03-30 11:34 ` [PATCH v3 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
@ 2021-03-30 11:34 ` Ciara Loftus
2021-03-30 15:08 ` Alexei Starovoitov
2021-03-30 11:34 ` [PATCH v3 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
2 siblings, 1 reply; 7+ messages in thread
From: Ciara Loftus @ 2021-03-30 11:34 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 | 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure
2021-03-30 11:34 ` [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
@ 2021-03-30 15:08 ` Alexei Starovoitov
2021-03-30 17:21 ` Björn Töpel
0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2021-03-30 15:08 UTC (permalink / raw)
To: Ciara Loftus; +Cc: Network Development, bpf, Karlsson, Magnus, bjorn
On Tue, Mar 30, 2021 at 5:06 AM 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 | 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));
> + }
The whole function increases indent, since it changes anyway
could you write it as:
{
if (--ctx->refcount)
return;
if (!unmap)
goto out_free;
err = xsk_get
if (err)
goto out_free;
munmap();
out_free:
list_del
free
}
other than this it looks fine to me.
Bjorn, Magnus,
please review.
> }
>
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure
2021-03-30 15:08 ` Alexei Starovoitov
@ 2021-03-30 17:21 ` Björn Töpel
0 siblings, 0 replies; 7+ messages in thread
From: Björn Töpel @ 2021-03-30 17:21 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Ciara Loftus, Network Development, bpf, Karlsson, Magnus
On Tue, 30 Mar 2021 at 17:08, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 5:06 AM Ciara Loftus <ciara.loftus@intel.com> wrote:
> >
[...]
> > 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));
> > + }
>
> The whole function increases indent, since it changes anyway
> could you write it as:
> {
> if (--ctx->refcount)
> return;
> if (!unmap)
> goto out_free;
> err = xsk_get
> if (err)
> goto out_free;
> munmap();
> out_free:
> list_del
> free
> }
>
Yes, please try to reduce the nesting, and while at it try to expand
the as much as possible of the munmap arguments to the full 100 chars.
Björn
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary
2021-03-30 11:34 [PATCH v3 bpf 0/3] AF_XDP Socket Creation Fixes Ciara Loftus
2021-03-30 11:34 ` [PATCH v3 bpf 1/3] libbpf: ensure umem pointer is non-NULL before dereferencing Ciara Loftus
2021-03-30 11:34 ` [PATCH v3 bpf 2/3] libbpf: restore umem state after socket create failure Ciara Loftus
@ 2021-03-30 11:34 ` Ciara Loftus
2021-03-30 17:25 ` Björn Töpel
2 siblings, 1 reply; 7+ messages in thread
From: Ciara Loftus @ 2021-03-30 11:34 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 a new flag to struct xsk_umem, and
checking it before setting up the rings for sockets which share the fd
with the UMEM.
Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
tools/lib/bpf/xsk.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index d4991ddff05a..12110bba4cc0 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -14,6 +14,7 @@
#include <unistd.h>
#include <arpa/inet.h>
#include <asm/barrier.h>
+#include <linux/bitops.h>
#include <linux/compiler.h>
#include <linux/ethtool.h>
#include <linux/filter.h>
@@ -46,6 +47,9 @@
#define PF_XDP AF_XDP
#endif
+#define XDP_RX_RING_SETUP_DONE BIT(0)
+#define XDP_TX_RING_SETUP_DONE BIT(1)
+
enum xsk_prog {
XSK_PROG_FALLBACK,
XSK_PROG_REDIRECT_FLAGS,
@@ -59,6 +63,7 @@ struct xsk_umem {
int fd;
int refcount;
struct list_head ctx_list;
+ __u8 ring_setup_status;
};
struct xsk_ctx {
@@ -855,6 +860,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;
@@ -882,6 +888,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
}
} else {
xsk->fd = umem->fd;
+ rx_setup_done = umem->ring_setup_status & XDP_RX_RING_SETUP_DONE;
+ tx_setup_done = umem->ring_setup_status & XDP_TX_RING_SETUP_DONE;
}
ctx = xsk_get_ctx(umem, ifindex, queue_id);
@@ -900,7 +908,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));
@@ -908,8 +916,10 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
err = -errno;
goto out_put_ctx;
}
+ if (xsk->fd == umem->fd)
+ umem->ring_setup_status |= XDP_RX_RING_SETUP_DONE;
}
- 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));
@@ -917,6 +927,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
err = -errno;
goto out_put_ctx;
}
+ if (xsk->fd == umem->fd)
+ umem->ring_setup_status |= XDP_TX_RING_SETUP_DONE;
}
err = xsk_get_mmap_offsets(xsk->fd, &off);
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary
2021-03-30 11:34 ` [PATCH v3 bpf 3/3] libbpf: only create rx and tx XDP rings when necessary Ciara Loftus
@ 2021-03-30 17:25 ` Björn Töpel
0 siblings, 0 replies; 7+ messages in thread
From: Björn Töpel @ 2021-03-30 17:25 UTC (permalink / raw)
To: Ciara Loftus; +Cc: Netdev, bpf, Karlsson, Magnus, Alexei Starovoitov
On Tue, 30 Mar 2021 at 14:05, Ciara Loftus <ciara.loftus@intel.com> wrote:
>
> 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 a new flag to struct xsk_umem, and
> checking it before setting up the rings for sockets which share the fd
> with the UMEM.
>
> Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets")
>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
> tools/lib/bpf/xsk.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index d4991ddff05a..12110bba4cc0 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -14,6 +14,7 @@
> #include <unistd.h>
> #include <arpa/inet.h>
> #include <asm/barrier.h>
> +#include <linux/bitops.h>
> #include <linux/compiler.h>
> #include <linux/ethtool.h>
> #include <linux/filter.h>
> @@ -46,6 +47,9 @@
> #define PF_XDP AF_XDP
> #endif
>
> +#define XDP_RX_RING_SETUP_DONE BIT(0)
> +#define XDP_TX_RING_SETUP_DONE BIT(1)
> +
> enum xsk_prog {
> XSK_PROG_FALLBACK,
> XSK_PROG_REDIRECT_FLAGS,
> @@ -59,6 +63,7 @@ struct xsk_umem {
> int fd;
> int refcount;
> struct list_head ctx_list;
> + __u8 ring_setup_status;
Are we envisioning any more flags here? Otherwise, just a simple
bool/__u8 stating ring_setup_complete or ring_is_setup and just use
true/false and a simple check w/o bitwise and.
Björn
^ permalink raw reply [flat|nested] 7+ messages in thread