[PATCH bpf-next 0/4] selftests: xsk: fix TEST_MODE_SKB in xdpxceiver

Hi!

First of all, this might look incorrect as title has a 'fix' keyword and
is sent to bpf-next, but this is due to the fact that Andrii moved xsk
part of libbpf to selftests yesterday, so it's just not in the bpf tree
yet. Also, no new API to libbpf's xsk would be accepted, so that's the
only way to go with fixing xdpxceiver currently. Besides fixes (#2 and
#4), #1 is a small optimization to reduce times we query bpf_link
capability and #3 is a protection against the thing we're fixing here.

This set is about fixing TEST_MODE_SKB in xdpxceiver. Current function
for loading XDP prog independently from AF_XDP socket ignores the flags
from user, such as XDP_FLAGS_SKB_MODE, which makes it impossible to test
generic XDP.  TEST_MODE_SKB was running with XDP prog in native mode,
which is not a thing that we want.

Had it been correctly in the first place, we would see that
refcounting/deleting XSK socket had issues as well. We need to free BPF
resources only when context refcount drops to zero. If its higher then
it means that we should not touch prog/map as other sockets are still
active.

Thanks,
Maciej

Maciej Fijalkowski (4):
  selftests: xsk: avoid bpf_link probe for existing xsk
  selftests: xsk: introduce XDP prog load based on existing AF_XDP
    socket
  selftests: xsk: verify correctness of XDP prog attach point
  selftests: xsk: destroy BPF resources only when ctx refcount drops to
    0

 tools/testing/selftests/bpf/xdpxceiver.c | 19 ++++++++++++++++++-
 tools/testing/selftests/bpf/xsk.c        | 15 ++++++++++-----
 tools/testing/selftests/bpf/xsk.h        |  1 +
 3 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.27.0

Currently bpf_link probe is done for each call of xsk_socket__create().
For cases where xsk context was previously created and current socket
creation uses it, has_bpf_link will be overwritten, where it has already
been initialized.

Optimize this by moving the query to the xsk_create_ctx() so that when
xsk_get_ctx() finds a ctx then no further bpf_link probes are needed.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 tools/testing/selftests/bpf/xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
index eb50c3f336f8..fa13d2c44517 100644
--- a/tools/testing/selftests/bpf/xsk.c
+++ b/tools/testing/selftests/bpf/xsk.c

@@ -958,6 +958,7 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
 	ctx->fill = fill;
 	ctx->comp = comp;
 	list_add(&ctx->list, &umem->ctx_list);
+	ctx->has_bpf_link = xsk_probe_bpf_link();
 	return ctx;
 }
 

@@ -1059,7 +1060,6 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 		}
 	}
 	xsk->ctx = ctx;
-	xsk->ctx->has_bpf_link = xsk_probe_bpf_link();
 
 	if (rx && !rx_setup_done) {
 		err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING,

-- 
2.27.0

Currently, xsk_setup_xdp_prog() uses anonymous xsk_socket struct which
means that during xsk_create_bpf_link() call, xsk->config.xdp_flags is
always 0. This in turn means that from xdpxceiver it is impossible to
use xdpgeneric attachment, so since commit 3b22523bca02 ("selftests,
xsk: Fix bpf_res cleanup test") we were not testing SKB mode at all.

To fix this, introduce a function, called xsk_setup_xdp_prog_xsk(), that
will load XDP prog based on the existing xsk_socket, so that xsk
context's refcount is correctly bumped and flags from application side
are respected. Use this from xdpxceiver side so we get coverage of
generic and native XDP program attach points.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 tools/testing/selftests/bpf/xdpxceiver.c | 2 +-
 tools/testing/selftests/bpf/xsk.c        | 5 +++++
 tools/testing/selftests/bpf/xsk.h        | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
index 019c567b6b4e..c024aa91ea02 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.c
+++ b/tools/testing/selftests/bpf/xdpxceiver.c

@@ -1130,7 +1130,7 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 	if (!ifindex)
 		exit_with_error(errno);
 
-	ret = xsk_setup_xdp_prog(ifindex, &ifobject->xsk_map_fd);
+	ret = xsk_setup_xdp_prog_xsk(ifobject->xsk->xsk, &ifobject->xsk_map_fd);
 	if (ret)
 		exit_with_error(-ret);
 

diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
index fa13d2c44517..db911127720e 100644
--- a/tools/testing/selftests/bpf/xsk.c
+++ b/tools/testing/selftests/bpf/xsk.c

@@ -880,6 +880,11 @@ static int __xsk_setup_xdp_prog(struct xsk_socket *_xdp, int *xsks_map_fd)
 	return err;
 }
 
+int xsk_setup_xdp_prog_xsk(struct xsk_socket *xsk, int *xsks_map_fd)
+{
+	return __xsk_setup_xdp_prog(xsk, xsks_map_fd);
+}
+
 static struct xsk_ctx *xsk_get_ctx(struct xsk_umem *umem, int ifindex,
 				   __u32 queue_id)
 {

diff --git a/tools/testing/selftests/bpf/xsk.h b/tools/testing/selftests/bpf/xsk.h
index 915e7135337c..997723b0bfb2 100644
--- a/tools/testing/selftests/bpf/xsk.h
+++ b/tools/testing/selftests/bpf/xsk.h

@@ -269,6 +269,7 @@ struct xsk_umem_config {
 	__u32 flags;
 };
 
+int xsk_setup_xdp_prog_xsk(struct xsk_socket *xsk, int *xsks_map_fd);
 int xsk_setup_xdp_prog(int ifindex, int *xsks_map_fd);
 int xsk_socket__update_xskmap(struct xsk_socket *xsk, int xsks_map_fd);
 

-- 
2.27.0

To prevent the case we had previously where for TEST_MODE_SKB, XDP prog
was attached in native mode, call bpf_xdp_query() after loading prog and
make sure that attach_mode is as expected.

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 tools/testing/selftests/bpf/xdpxceiver.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
index c024aa91ea02..4c425a43e5b0 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.c
+++ b/tools/testing/selftests/bpf/xdpxceiver.c

@@ -1085,6 +1085,7 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 {
 	u64 umem_sz = ifobject->umem->num_frames * ifobject->umem->frame_size;
 	int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
+	LIBBPF_OPTS(bpf_xdp_query_opts, opts);
 	int ret, ifindex;
 	void *bufs;
 	u32 i;

@@ -1134,6 +1135,22 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 	if (ret)
 		exit_with_error(-ret);
 
+	ret = bpf_xdp_query(ifindex, ifobject->xdp_flags, &opts);
+	if (ret)
+		exit_with_error(-ret);
+
+	if (ifobject->xdp_flags & XDP_FLAGS_SKB_MODE) {
+		if (opts.attach_mode != XDP_ATTACHED_SKB) {
+			ksft_print_msg("ERROR: [%s] XDP prog not in SKB mode\n");
+			exit_with_error(-EINVAL);
+		}
+	} else if (ifobject->xdp_flags & XDP_FLAGS_DRV_MODE) {
+		if (opts.attach_mode != XDP_ATTACHED_DRV) {
+			ksft_print_msg("ERROR: [%s] XDP prog not in DRV mode\n");
+			exit_with_error(-EINVAL);
+		}
+	}
+
 	ret = xsk_socket__update_xskmap(ifobject->xsk->xsk, ifobject->xsk_map_fd);
 	if (ret)
 		exit_with_error(-ret);

-- 
2.27.0

Currently, xsk_socket__delete frees BPF resources regardless of ctx
refcount. Xdpxceiver has a test to verify whether underlying BPF
resources would not be wiped out after closing XSK socket that was bound
to interface with other active sockets. From library's xsk part
perspective it also means that the internal xsk context is shared and
its refcount is bumped accordingly.

After a switch to loading XDP prog based on previously opened XSK
socket, mentioned xdpxceiver test fails with:
not ok 16 [xdpxceiver.c:swap_xsk_resources:1334]: ERROR: 9/"Bad file descriptor

which means that in swap_xsk_resources(), xsk_socket__delete() released
xskmap which in turn caused a failure of xsk_socket__update_xskmap().

To fix this, when deleting socket, decrement ctx refcount before
releasing BPF resources and do so only when refcount dropped to 0 which
means there are no more active sockets for this ctx so BPF resources can
be freed safely.

Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 tools/testing/selftests/bpf/xsk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
index db911127720e..95ce5cd572bb 100644
--- a/tools/testing/selftests/bpf/xsk.c
+++ b/tools/testing/selftests/bpf/xsk.c

@@ -1156,8 +1156,6 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
 		goto out_mmap_tx;
 	}
 
-	ctx->prog_fd = -1;
-
 	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
 		err = __xsk_setup_xdp_prog(xsk, NULL);
 		if (err)

@@ -1238,7 +1236,10 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 
 	ctx = xsk->ctx;
 	umem = ctx->umem;
-	if (ctx->prog_fd != -1) {
+
+	xsk_put_ctx(ctx, true);
+
+	if (!ctx->refcount) {
 		xsk_delete_bpf_maps(xsk);
 		close(ctx->prog_fd);
 		if (ctx->has_bpf_link)

@@ -1257,7 +1258,6 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 		}
 	}
 
-	xsk_put_ctx(ctx, true);
 
 	umem->refcount--;
 	/* Do not close an fd that also has an associated umem connected

-- 
2.27.0

On Wed, Jun 29, 2022 at 4:38 PM Maciej Fijalkowski
[off-list ref] wrote:

Currently bpf_link probe is done for each call of xsk_socket__create().
For cases where xsk context was previously created and current socket
creation uses it, has_bpf_link will be overwritten, where it has already
been initialized.

Optimize this by moving the query to the xsk_create_ctx() so that when
xsk_get_ctx() finds a ctx then no further bpf_link probes are needed.

This would be a good optimization to libxdp too.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

On Wed, Jun 29, 2022 at 4:38 PM Maciej Fijalkowski
[off-list ref] wrote:

Currently, xsk_setup_xdp_prog() uses anonymous xsk_socket struct which
means that during xsk_create_bpf_link() call, xsk->config.xdp_flags is
always 0. This in turn means that from xdpxceiver it is impossible to
use xdpgeneric attachment, so since commit 3b22523bca02 ("selftests,
xsk: Fix bpf_res cleanup test") we were not testing SKB mode at all.

To fix this, introduce a function, called xsk_setup_xdp_prog_xsk(), that
will load XDP prog based on the existing xsk_socket, so that xsk
context's refcount is correctly bumped and flags from application side
are respected. Use this from xdpxceiver side so we get coverage of
generic and native XDP program attach points.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

On Wed, Jun 29, 2022 at 4:39 PM Maciej Fijalkowski
[off-list ref] wrote:

To prevent the case we had previously where for TEST_MODE_SKB, XDP prog
was attached in native mode, call bpf_xdp_query() after loading prog and
make sure that attach_mode is as expected.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

On Wed, Jun 29, 2022 at 4:39 PM Maciej Fijalkowski
[off-list ref] wrote:

Currently, xsk_socket__delete frees BPF resources regardless of ctx
refcount. Xdpxceiver has a test to verify whether underlying BPF
resources would not be wiped out after closing XSK socket that was bound
to interface with other active sockets. From library's xsk part
perspective it also means that the internal xsk context is shared and
its refcount is bumped accordingly.

After a switch to loading XDP prog based on previously opened XSK
socket, mentioned xdpxceiver test fails with:
not ok 16 [xdpxceiver.c:swap_xsk_resources:1334]: ERROR: 9/"Bad file descriptor

which means that in swap_xsk_resources(), xsk_socket__delete() released
xskmap which in turn caused a failure of xsk_socket__update_xskmap().

To fix this, when deleting socket, decrement ctx refcount before
releasing BPF resources and do so only when refcount dropped to 0 which
means there are no more active sockets for this ctx so BPF resources can
be freed safely.

Please fix this in libxdp too as the bug is present there also.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

On 6/30/22 11:58 AM, Magnus Karlsson wrote:

On Wed, Jun 29, 2022 at 4:39 PM Maciej Fijalkowski
[off-list ref] wrote:

quoted

Currently, xsk_socket__delete frees BPF resources regardless of ctx
refcount. Xdpxceiver has a test to verify whether underlying BPF
resources would not be wiped out after closing XSK socket that was bound
to interface with other active sockets. From library's xsk part
perspective it also means that the internal xsk context is shared and
its refcount is bumped accordingly.

After a switch to loading XDP prog based on previously opened XSK
socket, mentioned xdpxceiver test fails with:
not ok 16 [xdpxceiver.c:swap_xsk_resources:1334]: ERROR: 9/"Bad file descriptor

which means that in swap_xsk_resources(), xsk_socket__delete() released
xskmap which in turn caused a failure of xsk_socket__update_xskmap().

To fix this, when deleting socket, decrement ctx refcount before
releasing BPF resources and do so only when refcount dropped to 0 which
means there are no more active sockets for this ctx so BPF resources can
be freed safely.

Please fix this in libxdp too as the bug is present there also.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

[...]

quoted

@@ -1238,7 +1236,10 @@ void xsk_socket__delete(struct xsk_socket *xsk)

         ctx = xsk->ctx;
         umem = ctx->umem;
-       if (ctx->prog_fd != -1) {
+
+       xsk_put_ctx(ctx, true);
+
+       if (!ctx->refcount) {
                 xsk_delete_bpf_maps(xsk);
                 close(ctx->prog_fd);
                 if (ctx->has_bpf_link)

@@ -1257,7 +1258,6 @@ void xsk_socket__delete(struct xsk_socket *xsk)
                 }
         }

-       xsk_put_ctx(ctx, true);

         umem->refcount--;

Applied & also fixed up the double newline. Thanks everyone!