netdev - Re: [PATCH net-next v3 2/2] selftests/bpf: check if the global consumer of tx queue updates after send call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFvpNHqvZp0eishZ@boxer>
Date: Wed, 25 Jun 2025 14:19:00 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jason Xing <kerneljasonxing@...il.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <bjorn@...nel.org>, <magnus.karlsson@...el.com>,
	<jonathan.lemon@...il.com>, <sdf@...ichev.me>, <ast@...nel.org>,
	<daniel@...earbox.net>, <hawk@...nel.org>, <john.fastabend@...il.com>,
	<joe@...a.to>, <willemdebruijn.kernel@...il.com>, <bpf@...r.kernel.org>,
	<netdev@...r.kernel.org>, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 2/2] selftests/bpf: check if the global
 consumer of tx queue updates after send call

On Wed, Jun 25, 2025 at 06:10:14PM +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> The subtest sends 33 packets at one time on purpose to see if xsk
> exitting __xsk_generic_xmit() updates the global consumer of tx queue
> when reaching the max loop (max_tx_budget, 32 by default). The number 33
> can avoid xskq_cons_peek_desc() updates the consumer, to accurately
> check if the issue that the first patch resolves remains.
> 
> Speaking of the selftest implementation, it's not possible to use the
> normal validation_func to check if the issue happens because the whole
> send packets logic will call the sendto multiple times such that we're
> unable to detect in time.
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 30 ++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 0ced4026ee44..f7aa83706bc7 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -109,6 +109,8 @@
>  
>  #include <network_helpers.h>
>  
> +#define MAX_TX_BUDGET_DEFAULT 32

and what if in the future you would increase the generic xmit budget on
the system? it would be better to wait with test addition when you
introduce the setsockopt patch.

plus keep in mind that xskxceiver tests ZC drivers as well. so either we
should have a test that serves all modes or keep it for skb mode only.

> +
>  static bool opt_verbose;
>  static bool opt_print_tests;
>  static enum test_mode opt_mode = TEST_MODE_ALL;
> @@ -1323,7 +1325,8 @@ static int receive_pkts(struct test_spec *test)
>  	return TEST_PASS;
>  }
>  
> -static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, bool timeout)
> +static int __send_pkts(struct test_spec *test, struct ifobject *ifobject,
> +		       struct xsk_socket_info *xsk, bool timeout)
>  {
>  	u32 i, idx = 0, valid_pkts = 0, valid_frags = 0, buffer_len;
>  	struct pkt_stream *pkt_stream = xsk->pkt_stream;
> @@ -1437,9 +1440,21 @@ static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, b
>  	}
>  
>  	if (!timeout) {
> +		int prev_tx_consumer;
> +
> +		if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE))
> +			prev_tx_consumer = *xsk->tx.consumer;
> +
>  		if (complete_pkts(xsk, i))
>  			return TEST_FAILURE;
>  
> +		if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE)) {
> +			int delta = *xsk->tx.consumer - prev_tx_consumer;

hacking the data path logic for single test purpose is rather not good.
I am also not really sure if this deserves a standalone test case or could
we just introduce a check in data path in appropriate place.

> +
> +			if (delta != MAX_TX_BUDGET_DEFAULT)
> +				return TEST_FAILURE;
> +		}
> +
>  		usleep(10);
>  		return TEST_PASS;
>  	}
> @@ -1492,7 +1507,7 @@ static int send_pkts(struct test_spec *test, struct ifobject *ifobject)
>  				__set_bit(i, bitmap);
>  				continue;
>  			}
> -			ret = __send_pkts(ifobject, &ifobject->xsk_arr[i], timeout);
> +			ret = __send_pkts(test, ifobject, &ifobject->xsk_arr[i], timeout);
>  			if (ret == TEST_CONTINUE && !test->fail)
>  				continue;
>  
> @@ -2613,6 +2628,16 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
>  				   XSK_UMEM__LARGE_FRAME_SIZE * 2);
>  }
>  
> +static int testapp_tx_queue_consumer(struct test_spec *test)
> +{
> +	int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
> +
> +	pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
> +	test->ifobj_tx->xsk->batch_size = nr_packets;
> +
> +	return testapp_validate_traffic(test);
> +}
> +
>  static void run_pkt_test(struct test_spec *test)
>  {
>  	int ret;
> @@ -2723,6 +2748,7 @@ static const struct test_spec tests[] = {
>  	{.name = "XDP_ADJUST_TAIL_SHRINK_MULTI_BUFF", .test_func = testapp_adjust_tail_shrink_mb},
>  	{.name = "XDP_ADJUST_TAIL_GROW", .test_func = testapp_adjust_tail_grow},
>  	{.name = "XDP_ADJUST_TAIL_GROW_MULTI_BUFF", .test_func = testapp_adjust_tail_grow_mb},
> +	{.name = "TX_QUEUE_CONSUMER", .test_func = testapp_tx_queue_consumer},
>  	};
>  
>  static void print_tests(void)
> -- 
> 2.41.3
>