AF_XDP Socket Creation Fixes

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 <[email protected]>

---
 tools/lib/bpf/xsk.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

tools/lib/bpf/xsk.c +18 -16

index 443b0cfb45e8..5098d9e3b55a 100644
@@ -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);

tools/lib/bpf/xsk.c +4 -3

        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

Quoted 11 lines · 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")

tools/lib/bpf/xsk.c static struct xsk_ctx *xsk_get_ctx(struct

Quoted 11 lines

Signed-off-by Ciara Loftus <[email protected]>

---
 tools/lib/bpf/xsk.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

tools/lib/bpf/xsk.c +0 -0

index 443b0cfb45e8..5098d9e3b55a 100644
@@ -743,26 +743,30 @@ static struct xsk_ctx *xsk_get_ctx(struct

xsk_umem *umem, int ifindex,

tools/lib/bpf/xsk.c static struct xsk_ctx *xsk_create_ctx(struct

Quoted 47 lines

tools/lib/bpf/xsk.c +11 -14

        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 *

tools/lib/bpf/xsk.c +2 -0

+              sizeof(__u64));
+       munmap(ctx->comp->ring - off.cr.desc, off.cr.desc + umem-

config.comp_size *

tools/lib/bpf/xsk.c +5 -0

+              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,

tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket

Quoted 9 lines

tools/lib/bpf/xsk.c +0 -2

        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,

tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, · line 860

Quoted 5 lines

tools/lib/bpf/xsk.c +1 -0

        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.

tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket

Quoted 4 lines

tools/lib/bpf/xsk.c +0 -0

        if (!umem || !xsk_ptr || !(rx || tx))
                return -EFAULT;

@@ -994,6 +997,8 @@ int xsk_socket__create_shared(struct xsk_socket

**xsk_ptr,

tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket

Quoted 9 lines

tools/lib/bpf/xsk.c +2 -0

        }
 
        *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,

Quoted 20 lines

tools/lib/bpf/xsk.c +2 -3

                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

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

Quoted 14 lines · 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.

Quoted 5 lines

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 <[email protected]>