netdev - Re: [PATCH net-next 2/4] vsock/test: Add test for accept

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: <jep457tawmephttltjbohtqx57z63auoshgeolzhacz7j7rwra@z2uqfegja6dm>
Date: Fri, 13 Dec 2024 12:55:50 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] vsock/test: Add test for accept_queue
 memory leak

On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote:
>On 12/10/24 17:18, Stefano Garzarella wrote:
>> On Fri, Dec 06, 2024 at 07:34:52PM +0100, Michal Luczaj wrote:
>>> [...]
>>> +#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */
>>> +
>>> +static void test_stream_leak_acceptq_client(const struct test_opts *opts)
>>> +{
>>> +	struct sockaddr_vm addr = {
>>> +		.svm_family = AF_VSOCK,
>>> +		.svm_port = opts->peer_port,
>>> +		.svm_cid = opts->peer_cid
>>> +	};
>>> +	time_t tout;
>>> +	int fd;
>>> +
>>> +	tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC;
>>> +	do {
>>> +		control_writeulong(1);
>>
>> Can we use control_writeln() and control_expectln()?
>
>Please see below.
>
>>> +
>>> +		fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>>> +		if (fd < 0) {
>>> +			perror("socket");
>>> +			exit(EXIT_FAILURE);
>>> +		}
>>> +
>>
>> Do we need another control messages (server -> client) here to be sure
>> the server is listening?
>
>Ahh, I get your point.
>
>>> +		connect(fd, (struct sockaddr *)&addr, sizeof(addr));
>>
>> What about using `vsock_stream_connect` so you can remove a lot of
>> code from this function (e.g. sockaddr_vm, socket(), etc.)
>>
>> We only need to add `control_expectln("LISTENING")` in the server which
>> should also fix my previous comment.
>
>Sure, I followed your suggestion with
>
>	tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC;
>	do {
>		control_writeulong(RACE_CONTINUE);
>		fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>		if (fd >= 0)
>			close(fd);

I'd do
		if (fd < 0) {
			perror("connect");
			exit(EXIT_FAILURE);
		}
		close(fd);

apart of that LGTM!

>	} while (current_nsec() < tout);
>	control_writeulong(RACE_DONE);
>
>vs.
>
>	while (control_readulong() == RACE_CONTINUE) {
>		fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
>		control_writeln("LISTENING");
>		close(fd);
>	}
>
>and it works just fine.
>
>>> +static void test_stream_leak_acceptq_server(const struct test_opts *opts)
>>> +{
>>> +	int fd;
>>> +
>>> +	while (control_readulong()) {
>>
>> Ah I see, the loop is easier by sending a number.
>> I would just add some comments when we send 1 and 0 to explain it.
>
>How about the #defines above?

yeah, I like it!

Thanks,
Stefano

>
>Thanks!
>Michal
>