netdev - Re: [PATCH] selftests: fix OOM problem in msg

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: <66831e1c3352a_46fc1294d8@willemb.c.googlers.com.notmuch>
Date: Mon, 01 Jul 2024 17:22:36 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: zijianzhang@...edance.com, 
 netdev@...r.kernel.org
Cc: edumazet@...gle.com, 
 willemdebruijn.kernel@...il.com, 
 cong.wang@...edance.com, 
 xiaochun.lu@...edance.com, 
 Zijian Zhang <zijianzhang@...edance.com>
Subject: Re: [PATCH] selftests: fix OOM problem in msg_zerocopy selftest

zijianzhang@ wrote:
> From: Zijian Zhang <zijianzhang@...edance.com>


Remember to append to PATCH net or net-next in the subject line.

Since the title has fix in it, I suppose this should go to net.

As this is a test adjustment, I don't think it should go to stable.
Still, fixes need a Fixes: tag. The below referenced commit is not the
cause. Likely that sysctl could be set to a different value to trigger
this on older kernels too.

This has likely been present since the start of the test, so

Fixes: 07b65c5b31ce ("test: add msg_zerocopy test")

> In selftests/net/msg_zerocopy.c, it has a while loop keeps calling sendmsg
> on a socket with MSG_ZEROCOPY flag, and it will recv the notifications
> until the socket is not writable. Typically, it will start the receiving
> process after around 30+ sendmsgs. However, because of the commit
> dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale") the sender is
> always writable and does not get any chance to run recv notifications.
> The selftest always exits with OUT_OF_MEMORY because the memory used by
> opt_skb exceeds the core.sysctl_optmem_max.

Regardless of how large you set this sysctl, right? It is suggested to
increase it to at least 128KB.

> We introduce "cfg_notification_limit" to force sender to receive
> notifications after some number of sendmsgs. And, notifications may not
> come in order, because of the reason we present above.

Which reason?

> We have order
> checking code managed by cfg_verbose.
> 
> Signed-off-by: Zijian Zhang <zijianzhang@...edance.com>
> Signed-off-by: Xiaochun Lu <xiaochun.lu@...edance.com>
> ---
>  tools/testing/selftests/net/msg_zerocopy.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c
> index bdc03a2097e8..7ea5fb28c93d 100644
> --- a/tools/testing/selftests/net/msg_zerocopy.c
> +++ b/tools/testing/selftests/net/msg_zerocopy.c
> @@ -85,6 +85,7 @@ static bool cfg_rx;
>  static int  cfg_runtime_ms	= 4200;
>  static int  cfg_verbose;
>  static int  cfg_waittime_ms	= 500;
> +static int  cfg_notification_limit = 32;
>  static bool cfg_zerocopy;
>  
>  static socklen_t cfg_alen;
> @@ -95,6 +96,7 @@ static char payload[IP_MAXPACKET];
>  static long packets, bytes, completions, expected_completions;
>  static int  zerocopied = -1;
>  static uint32_t next_completion;
> +static uint32_t sends_since_notify;
>  
>  static unsigned long gettimeofday_ms(void)
>  {
> @@ -208,6 +210,7 @@ static bool do_sendmsg(int fd, struct msghdr *msg, bool do_zerocopy, int domain)
>  		error(1, errno, "send");
>  	if (cfg_verbose && ret != len)
>  		fprintf(stderr, "send: ret=%u != %u\n", ret, len);
> +	sends_since_notify++;
>  
>  	if (len) {
>  		packets++;
> @@ -435,7 +438,7 @@ static bool do_recv_completion(int fd, int domain)
>  	/* Detect notification gaps. These should not happen often, if at all.
>  	 * Gaps can occur due to drops, reordering and retransmissions.
>  	 */
> -	if (lo != next_completion)
> +	if (cfg_verbose && lo != next_completion)
>  		fprintf(stderr, "gap: %u..%u does not append to %u\n",
>  			lo, hi, next_completion);
>  	next_completion = hi + 1;
> @@ -460,6 +463,7 @@ static bool do_recv_completion(int fd, int domain)
>  static void do_recv_completions(int fd, int domain)
>  {
>  	while (do_recv_completion(fd, domain)) {}
> +	sends_since_notify = 0;
>  }
>  
>  /* Wait for all remaining completions on the errqueue */
> @@ -549,6 +553,9 @@ static void do_tx(int domain, int type, int protocol)
>  		else
>  			do_sendmsg(fd, &msg, cfg_zerocopy, domain);
>  
> +		if (cfg_zerocopy && sends_since_notify >= cfg_notification_limit)
> +			do_recv_completions(fd, domain);
> +
>  		while (!do_poll(fd, POLLOUT)) {
>  			if (cfg_zerocopy)
>  				do_recv_completions(fd, domain);
> @@ -708,7 +715,7 @@ static void parse_opts(int argc, char **argv)
>  
>  	cfg_payload_len = max_payload_len;
>  
> -	while ((c = getopt(argc, argv, "46c:C:D:i:mp:rs:S:t:vz")) != -1) {
> +	while ((c = getopt(argc, argv, "46c:C:D:i:l:mp:rs:S:t:vz")) != -1) {
>  		switch (c) {
>  		case '4':
>  			if (cfg_family != PF_UNSPEC)
> @@ -736,6 +743,9 @@ static void parse_opts(int argc, char **argv)
>  			if (cfg_ifindex == 0)
>  				error(1, errno, "invalid iface: %s", optarg);
>  			break;
> +		case 'l':
> +			cfg_notification_limit = strtoul(optarg, NULL, 0);
> +			break;
>  		case 'm':
>  			cfg_cork_mixed = true;
>  			break;
> -- 
> 2.20.1
>