netdev - Re: [PATCH net-next 2/3] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <6615b264894a0_24a51429432@willemb.c.googlers.com.notmuch>
Date: Tue, 09 Apr 2024 17:25:56 -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, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 cong.wang@...edance.com, 
 xiaochun.lu@...edance.com, 
 Zijian Zhang <zijianzhang@...edance.com>
Subject: Re: [PATCH net-next 2/3] selftests: fix OOM problem in msg_zerocopy
 selftest

zijianzhang@ wrote:
> From: Zijian Zhang <zijianzhang@...edance.com>
> 
> In selftests/net/msg_zerocopy.c, it has a while loop keeps calling sendmsg
> on a socket, and it will recv the completion notifications when 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. We introduce "cfg_notification_limit" to
> force sender to receive notifications after some number of sendmsgs.

No need for a new option. Existing test automation will not enable
that.

I have not observed this behavior in tests (so I wonder what is
different about the setups). But it is fine to unconditionally force
a call to do_recv_completions every few sends.

> Plus,
> in the selftest, we need to update skb_orphan_frags_rx to be the same as
> skb_orphan_frags.

To be able to test over loopback, I suppose?

> In this case, for some reason, notifications do not
> come in order now. We introduce "cfg_notification_order_check" to
> possibly ignore the checking for order.

Were you testing UDP?

I don't think this is needed. I wonder what you were doing to see
enough of these events to want to suppress the log output.
 
> Signed-off-by: Zijian Zhang <zijianzhang@...edance.com>
> Signed-off-by: Xiaochun Lu <xiaochun.lu@...edance.com>
> ---
>  tools/testing/selftests/net/msg_zerocopy.c | 24 ++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c
> index bdc03a2097e8..8e595216a0af 100644
> --- a/tools/testing/selftests/net/msg_zerocopy.c
> +++ b/tools/testing/selftests/net/msg_zerocopy.c
> @@ -85,6 +85,8 @@ static bool cfg_rx;
>  static int  cfg_runtime_ms	= 4200;
>  static int  cfg_verbose;
>  static int  cfg_waittime_ms	= 500;
> +static bool cfg_notification_order_check;
> +static int  cfg_notification_limit = 32;
>  static bool cfg_zerocopy;
>  
>  static socklen_t cfg_alen;
> @@ -435,7 +437,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_notification_order_check && lo != next_completion)
>  		fprintf(stderr, "gap: %u..%u does not append to %u\n",
>  			lo, hi, next_completion);
>  	next_completion = hi + 1;
> @@ -489,7 +491,7 @@ static void do_tx(int domain, int type, int protocol)
>  		struct iphdr iph;
>  	} nh;
>  	uint64_t tstop;
> -	int fd;
> +	int fd, sendmsg_counter = 0;
>  
>  	fd = do_setup_tx(domain, type, protocol);
>  
> @@ -548,10 +550,18 @@ static void do_tx(int domain, int type, int protocol)
>  			do_sendmsg_corked(fd, &msg);
>  		else
>  			do_sendmsg(fd, &msg, cfg_zerocopy, domain);
> +		sendmsg_counter++;
> +
> +		if (sendmsg_counter == cfg_notification_limit && cfg_zerocopy) {
> +			do_recv_completions(fd, domain);
> +			sendmsg_counter = 0;
> +		}
>  
>  		while (!do_poll(fd, POLLOUT)) {
> -			if (cfg_zerocopy)
> +			if (cfg_zerocopy) {
>  				do_recv_completions(fd, domain);
> +				sendmsg_counter = 0;
> +			}
>  		}
>  
>  	} while (gettimeofday_ms() < tstop);
> @@ -708,7 +718,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:mp:rs:S:t:vzol:")) != -1) {
>  		switch (c) {
>  		case '4':
>  			if (cfg_family != PF_UNSPEC)
> @@ -760,6 +770,12 @@ static void parse_opts(int argc, char **argv)
>  		case 'z':
>  			cfg_zerocopy = true;
>  			break;
> +		case 'o':
> +			cfg_notification_order_check = true;
> +			break;
> +		case 'l':
> +			cfg_notification_limit = strtoul(optarg, NULL, 0);
> +			break;
>  		}
>  	}
>  
> -- 
> 2.20.1
>