AF_XDP Socket Creation Fixes
Subsystems (4)
v2 cover AF_XDP Socket Creation Fixes
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(-)
Signature · 1 line
2.17.1
PATCH v2 1/3 libbpf: ensure umem pointer is non-NULL before dereferencing
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.
--- tools/lib/bpf/xsk.c | 3 +++ 1 file changed, 3 insertions(+)
tools/lib/bpf/xsk.c +3 -0
@@ -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);
Signature · 1 line
2.17.1
PATCH v2 2/3 libbpf: restore umem state after socket create failure
In reply to Ciara LoftusIf 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.
--- tools/lib/bpf/xsk.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
tools/lib/bpf/xsk.c +16 -13
@@ -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
Signature · 1 line
2.17.1
PATCH v2 3/3 libbpf: ignore return values of setsockopt for XDP rings.
In reply to Ciara LoftusDuring 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.
--- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
tools/lib/bpf/xsk.c +16 -18
@@ -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) {
Signature · 1 line
2.17.1
Re: [PATCH v2 bpf 3/3] libbpf: ignore return values of setsockopt for XDP rings.
Quoted 49 lines · Ciara Loftus
During
xsk_socket__createtheXDP_RX_RINGandXDP_TX_RINGsetsockopts are called to create the rx and tx rings for theAF_XDPsocket. If the ring has already been set up, the setsockopt will return an error. However, in the event of a failure duringxsk_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 tommap()will catch it.Fixes 1cad07884239 ("libbpf: add support for using AF_XDP sockets")Acked-by Magnus Karlsson <[email protected]>Signed-off-by Ciara Loftus <[email protected]>--- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)tools/lib/bpf/xsk.c +6
-18@@ -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.
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, · line 918
Quoted 10 lines
tools/lib/bpf/xsk.c +10
-0+*/ +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.
Quoted 57 lines · Ciara Loftus
Quoted 1 line
During
xsk_socket__createtheXDP_RX_RINGandXDP_TX_RINGsetsockopts
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket
Quoted 21 lines
are called to create the rx and tx rings for the
AF_XDPsocket. If the ring has already been set up, the setsockopt will return an error. However, in the event of a failure duringxsk_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 tommap()will catch it.Fixes 1cad07884239 ("libbpf: add support for using AF_XDP sockets")Acked-by Magnus Karlsson <[email protected]>Signed-off-by Ciara Loftus <[email protected]>--- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)tools/lib/bpf/xsk.c +0
-0@@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket**xsk_ptr,
Quoted 12 lines
tools/lib/bpf/xsk.c +0
-9} 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; - } - }tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
Quoted 10 lines
tools/lib/bpf/xsk.c +1
-9-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.
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 904
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* If the ring has already been set up setsockopt will return an error. However,
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 905
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* this scenario is acceptable as the user may be retrying the socket creation
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 906
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* with rings which were set up in a previous but ultimately unsuccessful call
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 907
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* to xsk_socket__create(_shared). The call later to mmap() will fail if there
Quoted 14 lines
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, · line 918
Quoted 11 lines
tools/lib/bpf/xsk.c +11
-0+* 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_socketand 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.
Quoted 70 lines · Ciara
Quoted 70 lines · Ciara Loftus
Quoted 1 line
During
xsk_socket__createtheXDP_RX_RINGandXDP_TX_RINGsetsockopts
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket
Quoted 21 lines
are called to create the rx and tx rings for the
AF_XDPsocket. If the ring has already been set up, the setsockopt will return an error. However, in the event of a failure duringxsk_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 tommap()will catch it.Fixes 1cad07884239 ("libbpf: add support for using AF_XDP sockets")Acked-by Magnus Karlsson <[email protected]>Signed-off-by Ciara Loftus <[email protected]>--- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)tools/lib/bpf/xsk.c +0
-0@@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket**xsk_ptr,
Quoted 12 lines
tools/lib/bpf/xsk.c +0
-9} 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; - } - }tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
Quoted 10 lines
tools/lib/bpf/xsk.c +1
-9-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.
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 904
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* If the ring has already been set up setsockopt will return an error. However,
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 905
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* this scenario is acceptable as the user may be retrying the socket creation
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 906
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* with rings which were set up in a previous but ultimately unsuccessful call
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket · line 907
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* to xsk_socket__create(_shared). The call later to mmap() will fail if there
Quoted 11 lines
tools/lib/bpf/xsk.c +11
-0+* 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));
Quoted 4 lines
Quoted 2 lines
Instead of ignoring the error can you remember that setsockopt was done in struct
xsk_socketand don't do it the second time?Ideally we don't have to ignore the error. However in the event of failure struct
xsk_socketis freed at the end ofxsk_socket__createso 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.
Quoted 72 lines
Quoted 21 lines
Quoted 21 lines · Ciara Loftus
Quoted 1 line
During
xsk_socket__createtheXDP_RX_RINGandXDP_TX_RINGsetsockopts
Quoted 16 lines
are called to create the rx and tx rings for the
AF_XDPsocket. If the ring has already been set up, the setsockopt will return an error. However, in the event of a failure duringxsk_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 tommap()will catch it.Fixes 1cad07884239 ("libbpf: add support for using AF_XDP sockets")Acked-by Magnus Karlsson <[email protected]>Signed-off-by Ciara Loftus <[email protected]>--- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)Quoted 5 lines
Quoted 5 lines
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct
Quoted 5 lines
tools/lib/bpf/xsk.c +0
-0@@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct
xsk_socketQuoted 44 lines
Quoted 44 lines
**xsk_ptr,
Quoted 12 lines
tools/lib/bpf/xsk.c +0
-9} 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; - } - }tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
Quoted 10 lines
tools/lib/bpf/xsk.c +1
-9-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.
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct · line 904
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* If the ring has already been set up setsockopt will return an error. However,
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct · line 905
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* this scenario is acceptable as the user may be retrying the socket creation
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct · line 906
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* with rings which were set up in a previous but ultimately unsuccessful call
tools/lib/bpf/xsk.c int xsk_socket__create_shared(struct · line 907
Quoted 1 line
tools/lib/bpf/xsk.c +1
-0+* to xsk_socket__create(_shared). The call later to mmap() will fail if there
Quoted 11 lines
tools/lib/bpf/xsk.c +11
-0+* 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));Quoted 1 line
Quoted 1 line
Instead of ignoring the error can you remember that setsockopt was
Quoted 8 lines
done
Quoted 3 lines
Quoted 1 line
in struct
xsk_socketand don't do it the second time?Ideally we don't have to ignore the error. However in the event of failure
struct
xsk_socketis freed at the end ofxsk_socket__createso 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