[PATCH bpf 0/3] selftests/xsk: three small fixes

[PATCH bpf 0/3] selftests/xsk: three small fixes

[Magnus Karlsson]

This patch set contains three small fixes for bugs discovered while
implementing some new tests that will be sent to bpf-next. These are
minor fixes to the xsk test framework that IMHO should not be
backported to any of the stable kernels as no one using them would
care. The tests work fine without these fixes, but when developing new
code and some test fails (or the framework fails), these bugs in the
code are irritating.

Thanks: Magnus

Magnus Karlsson (3):
  selftests/xsk: print correct payload for packet dump
  selftests/xsk: do not close unused file descriptors
  selftests/xsk: print correct error codes when exiting

 tools/testing/selftests/bpf/xsk.c        | 13 +++++++++++--
 tools/testing/selftests/bpf/xskxceiver.c | 24 ++++++++++++------------
 2 files changed, 23 insertions(+), 14 deletions(-)


base-commit: 5fd2a60aecf3a42b14fa371c55b3dbb18b229230
--
2.34.1

[PATCH bpf 1/3] selftests/xsk: print correct payload for packet dump

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Print the correct payload when the packet dump option is selected. The
network to host conversion was forgotten and the payload was
erronously declared to be an int instead of an unsigned int. Changed
the loop index i too, as it does not need to be an int and was
declared on the same row.

The printout looks something like this after the fix:

DEBUG>> L2: dst mac: 000A569EEE62
DEBUG>> L2: src mac: 000A569EEE61
DEBUG>> L3: ip_hdr->ihl: 05
DEBUG>> L3: ip_hdr->saddr: 192.168.100.161
DEBUG>> L3: ip_hdr->daddr: 192.168.100.162
DEBUG>> L4: udp_hdr->src: 2121
DEBUG>> L4: udp_hdr->dst: 2020
DEBUG>> L5: payload: 4
---------------------------------------

Fixes: facb7cb2e909 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/testing/selftests/bpf/xskxceiver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 681a5db80dae..51e693318b3f 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -767,7 +767,7 @@ static void pkt_dump(void *pkt, u32 len)
 	struct ethhdr *ethhdr;
 	struct udphdr *udphdr;
 	struct iphdr *iphdr;
-	int payload, i;
+	u32 payload, i;
 
 	ethhdr = pkt;
 	iphdr = pkt + sizeof(*ethhdr);
@@ -792,7 +792,7 @@ static void pkt_dump(void *pkt, u32 len)
 	fprintf(stdout, "DEBUG>> L4: udp_hdr->src: %d\n", ntohs(udphdr->source));
 	fprintf(stdout, "DEBUG>> L4: udp_hdr->dst: %d\n", ntohs(udphdr->dest));
 	/*extract L5 frame */
-	payload = *((uint32_t *)(pkt + PKT_HDR_SIZE));
+	payload = ntohl(*((u32 *)(pkt + PKT_HDR_SIZE)));
 
 	fprintf(stdout, "DEBUG>> L5: payload: %d\n", payload);
 	fprintf(stdout, "---------------------------------------\n");
-- 
2.34.1

[PATCH bpf 2/3] selftests/xsk: do not close unused file descriptors

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Do not close descriptors that have never been used. File descriptor
fields that are not in use are erroneously marked with the number 0,
which is a valid fd. Mark unused fds with -1 instead and do not close
these when deleting the socket.

Fixes: f36600634282 ("libbpf: move xsk.{c,h} into selftests/bpf")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/testing/selftests/bpf/xsk.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
index 0b3ff49c740d..bea4cd076cd4 100644
--- a/tools/testing/selftests/bpf/xsk.c
+++ b/tools/testing/selftests/bpf/xsk.c
@@ -34,6 +34,8 @@
 #include <bpf/libbpf.h>
 #include "xsk.h"
 
+#define FD_NOT_USED (-1)
+
 #ifndef SOL_XDP
  #define SOL_XDP 283
 #endif
@@ -601,6 +603,9 @@ static void xsk_delete_bpf_maps(struct xsk_socket *xsk)
 {
 	struct xsk_ctx *ctx = xsk->ctx;
 
+	if (ctx->xsks_map_fd == FD_NOT_USED)
+		return;
+
 	bpf_map_delete_elem(ctx->xsks_map_fd, &ctx->queue_id);
 	close(ctx->xsks_map_fd);
 }
@@ -959,6 +964,9 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk,
 	ctx->umem = umem;
 	ctx->queue_id = queue_id;
 	libbpf_strlcpy(ctx->ifname, ifname, IFNAMSIZ);
+	ctx->prog_fd = FD_NOT_USED;
+	ctx->link_fd = FD_NOT_USED;
+	ctx->xsks_map_fd = FD_NOT_USED;
 
 	ctx->fill = fill;
 	ctx->comp = comp;
@@ -1239,8 +1247,9 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 
 	if (ctx->refcount == 1) {
 		xsk_delete_bpf_maps(xsk);
-		close(ctx->prog_fd);
-		if (ctx->has_bpf_link)
+		if (ctx->prog_fd != FD_NOT_USED)
+			close(ctx->prog_fd);
+		if (ctx->has_bpf_link && ctx->link_fd != FD_NOT_USED)
 			close(ctx->link_fd);
 	}
 
-- 
2.34.1

[PATCH bpf 3/3] selftests/xsk: print correct error codes when exiting

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Print the correct error codes when exiting the test suite due to some
terminal error. Some of these had a switched sign and some of them
printed zero instead of errno.

Fixes: facb7cb2e909 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/testing/selftests/bpf/xskxceiver.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 51e693318b3f..507dd28801fa 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -350,7 +350,7 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
 	umem = calloc(1, sizeof(struct xsk_umem_info));
 	if (!umem) {
 		munmap(bufs, umem_sz);
-		exit_with_error(-ENOMEM);
+		exit_with_error(ENOMEM);
 	}
 	umem->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
 	ret = xsk_configure_umem(umem, bufs, umem_sz);
@@ -936,7 +936,7 @@ static int receive_pkts(struct test_spec *test, struct pollfd *fds)
 		if (ifobj->use_poll) {
 			ret = poll(fds, 1, POLL_TMOUT);
 			if (ret < 0)
-				exit_with_error(-ret);
+				exit_with_error(errno);
 
 			if (!ret) {
 				if (!is_umem_valid(test->ifobj_tx))
@@ -963,7 +963,7 @@ static int receive_pkts(struct test_spec *test, struct pollfd *fds)
 				if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
 					ret = poll(fds, 1, POLL_TMOUT);
 					if (ret < 0)
-						exit_with_error(-ret);
+						exit_with_error(errno);
 				}
 				ret = xsk_ring_prod__reserve(&umem->fq, rcvd, &idx_fq);
 			}
@@ -1014,7 +1014,7 @@ static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, struct pollfd *fd
 			if (timeout) {
 				if (ret < 0) {
 					ksft_print_msg("ERROR: [%s] Poll error %d\n",
-						       __func__, ret);
+						       __func__, errno);
 					return TEST_FAILURE;
 				}
 				if (ret == 0)
@@ -1023,7 +1023,7 @@ static int __send_pkts(struct ifobject *ifobject, u32 *pkt_nb, struct pollfd *fd
 			}
 			if (ret <= 0) {
 				ksft_print_msg("ERROR: [%s] Poll error %d\n",
-					       __func__, ret);
+					       __func__, errno);
 				return TEST_FAILURE;
 			}
 		}
@@ -1317,23 +1317,23 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 
 	ret = bpf_xdp_query(ifindex, ifobject->xdp_flags, &opts);
 	if (ret)
-		exit_with_error(-ret);
+		exit_with_error(errno);
 
 	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);
+			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);
+			exit_with_error(EINVAL);
 		}
 	}
 
 	ret = xsk_socket__update_xskmap(ifobject->xsk->xsk, ifobject->xsk_map_fd);
 	if (ret)
-		exit_with_error(-ret);
+		exit_with_error(errno);
 }
 
 static void *worker_testapp_validate_tx(void *arg)
@@ -1540,7 +1540,7 @@ static void swap_xsk_resources(struct ifobject *ifobj_tx, struct ifobject *ifobj
 
 	ret = xsk_socket__update_xskmap(ifobj_rx->xsk->xsk, ifobj_rx->xsk_map_fd);
 	if (ret)
-		exit_with_error(-ret);
+		exit_with_error(errno);
 }
 
 static void testapp_bpf_res(struct test_spec *test)
-- 
2.34.1

Re: [PATCH bpf 1/3] selftests/xsk: print correct payload for packet dump

[Maciej Fijalkowski]

On Tue, Nov 15, 2022 at 09:05:36AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> Print the correct payload when the packet dump option is selected. The
> network to host conversion was forgotten and the payload was
> erronously declared to be an int instead of an unsigned int. Changed
> the loop index i too, as it does not need to be an int and was
> declared on the same row.
> 
> The printout looks something like this after the fix:
> 
> DEBUG>> L2: dst mac: 000A569EEE62
> DEBUG>> L2: src mac: 000A569EEE61
> DEBUG>> L3: ip_hdr->ihl: 05
> DEBUG>> L3: ip_hdr->saddr: 192.168.100.161
> DEBUG>> L3: ip_hdr->daddr: 192.168.100.162
> DEBUG>> L4: udp_hdr->src: 2121
> DEBUG>> L4: udp_hdr->dst: 2020
> DEBUG>> L5: payload: 4
> ---------------------------------------

Above would be helpful if previous output was included as well but not a
big deal i guess.

> 
> Fixes: facb7cb2e909 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 681a5db80dae..51e693318b3f 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -767,7 +767,7 @@ static void pkt_dump(void *pkt, u32 len)
>  	struct ethhdr *ethhdr;
>  	struct udphdr *udphdr;
>  	struct iphdr *iphdr;
> -	int payload, i;
> +	u32 payload, i;
>  
>  	ethhdr = pkt;
>  	iphdr = pkt + sizeof(*ethhdr);
> @@ -792,7 +792,7 @@ static void pkt_dump(void *pkt, u32 len)
>  	fprintf(stdout, "DEBUG>> L4: udp_hdr->src: %d\n", ntohs(udphdr->source));
>  	fprintf(stdout, "DEBUG>> L4: udp_hdr->dst: %d\n", ntohs(udphdr->dest));
>  	/*extract L5 frame */
> -	payload = *((uint32_t *)(pkt + PKT_HDR_SIZE));
> +	payload = ntohl(*((u32 *)(pkt + PKT_HDR_SIZE)));
>  
>  	fprintf(stdout, "DEBUG>> L5: payload: %d\n", payload);
>  	fprintf(stdout, "---------------------------------------\n");
> -- 
> 2.34.1
> 

Re: [PATCH bpf 0/3] selftests/xsk: three small fixes

[Maciej Fijalkowski]

On Tue, Nov 15, 2022 at 09:05:35AM +0100, Magnus Karlsson wrote:
> This patch set contains three small fixes for bugs discovered while
> implementing some new tests that will be sent to bpf-next. These are
> minor fixes to the xsk test framework that IMHO should not be
> backported to any of the stable kernels as no one using them would
> care. The tests work fine without these fixes, but when developing new
> code and some test fails (or the framework fails), these bugs in the
> code are irritating.
> 
> Thanks: Magnus

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> Magnus Karlsson (3):
>   selftests/xsk: print correct payload for packet dump
>   selftests/xsk: do not close unused file descriptors
>   selftests/xsk: print correct error codes when exiting
> 
>  tools/testing/selftests/bpf/xsk.c        | 13 +++++++++++--
>  tools/testing/selftests/bpf/xskxceiver.c | 24 ++++++++++++------------
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> 
> base-commit: 5fd2a60aecf3a42b14fa371c55b3dbb18b229230
> --
> 2.34.1

Re: [PATCH bpf 1/3] selftests/xsk: print correct payload for packet dump

[Magnus Karlsson]

On Tue, Nov 15, 2022 at 1:29 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Nov 15, 2022 at 09:05:36AM +0100, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Print the correct payload when the packet dump option is selected. The
> > network to host conversion was forgotten and the payload was
> > erronously declared to be an int instead of an unsigned int. Changed
> > the loop index i too, as it does not need to be an int and was
> > declared on the same row.
> >
> > The printout looks something like this after the fix:
> >
> > DEBUG>> L2: dst mac: 000A569EEE62
> > DEBUG>> L2: src mac: 000A569EEE61
> > DEBUG>> L3: ip_hdr->ihl: 05
> > DEBUG>> L3: ip_hdr->saddr: 192.168.100.161
> > DEBUG>> L3: ip_hdr->daddr: 192.168.100.162
> > DEBUG>> L4: udp_hdr->src: 2121
> > DEBUG>> L4: udp_hdr->dst: 2020
> > DEBUG>> L5: payload: 4
> > ---------------------------------------
>
> Above would be helpful if previous output was included as well but not a
> big deal i guess.

It would not bring any value IMHO. The only difference is that the
"L5: payload" row is now showing the correct payload.

> >
> > Fixes: facb7cb2e909 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL")
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > index 681a5db80dae..51e693318b3f 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -767,7 +767,7 @@ static void pkt_dump(void *pkt, u32 len)
> >       struct ethhdr *ethhdr;
> >       struct udphdr *udphdr;
> >       struct iphdr *iphdr;
> > -     int payload, i;
> > +     u32 payload, i;
> >
> >       ethhdr = pkt;
> >       iphdr = pkt + sizeof(*ethhdr);
> > @@ -792,7 +792,7 @@ static void pkt_dump(void *pkt, u32 len)
> >       fprintf(stdout, "DEBUG>> L4: udp_hdr->src: %d\n", ntohs(udphdr->source));
> >       fprintf(stdout, "DEBUG>> L4: udp_hdr->dst: %d\n", ntohs(udphdr->dest));
> >       /*extract L5 frame */
> > -     payload = *((uint32_t *)(pkt + PKT_HDR_SIZE));
> > +     payload = ntohl(*((u32 *)(pkt + PKT_HDR_SIZE)));
> >
> >       fprintf(stdout, "DEBUG>> L5: payload: %d\n", payload);
> >       fprintf(stdout, "---------------------------------------\n");
> > --
> > 2.34.1
> >

Re: [PATCH bpf 1/3] selftests/xsk: print correct payload for packet dump

[Maciej Fijalkowski]

On Tue, Nov 15, 2022 at 01:40:48PM +0100, Magnus Karlsson wrote:
> On Tue, Nov 15, 2022 at 1:29 PM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Tue, Nov 15, 2022 at 09:05:36AM +0100, Magnus Karlsson wrote:
> > > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > >
> > > Print the correct payload when the packet dump option is selected. The
> > > network to host conversion was forgotten and the payload was
> > > erronously declared to be an int instead of an unsigned int. Changed
> > > the loop index i too, as it does not need to be an int and was
> > > declared on the same row.
> > >
> > > The printout looks something like this after the fix:
> > >
> > > DEBUG>> L2: dst mac: 000A569EEE62
> > > DEBUG>> L2: src mac: 000A569EEE61
> > > DEBUG>> L3: ip_hdr->ihl: 05
> > > DEBUG>> L3: ip_hdr->saddr: 192.168.100.161
> > > DEBUG>> L3: ip_hdr->daddr: 192.168.100.162
> > > DEBUG>> L4: udp_hdr->src: 2121
> > > DEBUG>> L4: udp_hdr->dst: 2020
> > > DEBUG>> L5: payload: 4
> > > ---------------------------------------
> >
> > Above would be helpful if previous output was included as well but not a
> > big deal i guess.
> 
> It would not bring any value IMHO. The only difference is that the
> "L5: payload" row is now showing the correct payload.

Ah okay then. I have already acked whole series, but just to make things
clear, I am okay with the current state of this patch:

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> > >
> > > Fixes: facb7cb2e909 ("selftests/bpf: Xsk selftests - SKB POLL, NOPOLL")
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > ---
> > >  tools/testing/selftests/bpf/xskxceiver.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > > index 681a5db80dae..51e693318b3f 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > @@ -767,7 +767,7 @@ static void pkt_dump(void *pkt, u32 len)
> > >       struct ethhdr *ethhdr;
> > >       struct udphdr *udphdr;
> > >       struct iphdr *iphdr;
> > > -     int payload, i;
> > > +     u32 payload, i;
> > >
> > >       ethhdr = pkt;
> > >       iphdr = pkt + sizeof(*ethhdr);
> > > @@ -792,7 +792,7 @@ static void pkt_dump(void *pkt, u32 len)
> > >       fprintf(stdout, "DEBUG>> L4: udp_hdr->src: %d\n", ntohs(udphdr->source));
> > >       fprintf(stdout, "DEBUG>> L4: udp_hdr->dst: %d\n", ntohs(udphdr->dest));
> > >       /*extract L5 frame */
> > > -     payload = *((uint32_t *)(pkt + PKT_HDR_SIZE));
> > > +     payload = ntohl(*((u32 *)(pkt + PKT_HDR_SIZE)));
> > >
> > >       fprintf(stdout, "DEBUG>> L5: payload: %d\n", payload);
> > >       fprintf(stdout, "---------------------------------------\n");
> > > --
> > > 2.34.1
> > >