netdev - Re: [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to

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: <027fdb41-ee11-4be0-a493-22f28a1abd7c@rbox.co>
Date: Mon, 22 Jul 2024 15:07:28 +0200
From: Michal Luczaj <mhal@...x.co>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 john.fastabend@...il.com, kuniyu@...zon.com, Rao.Shoaib@...cle.com,
 cong.wang@...edance.com
Subject: Re: [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in
 unix_inet_redir_to_connected()

On 7/19/24 13:09, Jakub Sitnicki wrote:
> On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
>> On 7/13/24 11:45, Jakub Sitnicki wrote:
>>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>>> And looking at that commit[1], inet_unix_redir_to_connected() has its
>>>> @type ignored, too.  Same treatment?
>>>
>>> That one will not be a trivial fix like this case. inet_socketpair()
>>> won't work for TCP as is. It will fail trying to connect() a listening
>>> socket (p0). I recall now that we are in this state due to some
>>> abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
>>> inet in some function names").
>>> [...]
>>
>> Is this what you've meant? With this patch inet_socketpair() and
>> vsock_socketpair_connectible can be reduced to a single call to
>> create_pair(). And pairs creation in inet_unix_redir_to_connected()
>> and unix_inet_redir_to_connected() accepts both sotypes.
> 
> Yes, exactly. This looks great.

Happy to hear that. I'll prepare a series, include the little fixes and
send it out for a proper review.

One more thing: I've noticed changes in sockmap_helpers.h don't trigger
test_progs rebuild (seems to be the case for all .h in prog_tests/). No
idea if this is the right approach, but adding
"$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
selftests/bpf/Makefile does the trick.

> Classic cleanup with goto to close sockets is all right, but if you're
> feeling brave and aim for something less branchy, I've noticed we have
> finally started using __attribute__((cleanup)):
> 
> https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115

I've tried. Is such "ownership passing" (to inhibit the cleanup) via
construct like take_fd()[1] welcomed?

[1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/

static inline void close_fd(int *fd)
{
	if (*fd >= 0)
		xclose(*fd);
}

#define __closefd __attribute__((cleanup(close_fd)))

static inline int create_pair(int family, int sotype, int *c, int *p)
{
	struct sockaddr_storage addr;
	socklen_t len = sizeof(addr);
	int err;

	int s __closefd = socket_loopback(family, sotype);
	if (s < 0)
		return s;

	err = xgetsockname(s, sockaddr(&addr), &len);
	if (err)
		return err;

	int s0 __closefd = xsocket(family, sotype, 0);
	if (s0 < 0)
		return s0;

	err = connect(s0, sockaddr(&addr), len);
	if (err) {
		if (errno != EINPROGRESS) {
			FAIL_ERRNO("connect");
			return err;
		}

		err = poll_connect(s0, IO_TIMEOUT_SEC);
		if (err) {
			FAIL_ERRNO("poll_connect");
			return err;
		}
	}

	switch (sotype & SOCK_TYPE_MASK) {
	case SOCK_DGRAM:
		err = xgetsockname(s0, sockaddr(&addr), &len);
		if (err)
			return err;

		err = xconnect(s, sockaddr(&addr), len);
		if (err)
			return err;

		*p = take_fd(s);
		break;
	case SOCK_STREAM:
	case SOCK_SEQPACKET:
		*p = xaccept_nonblock(s, NULL, NULL);
		if (*p < 0)
			return *p;
		break;
	default:
		FAIL("Unsupported socket type %#x", sotype);
		return -EOPNOTSUPP;
	}

	*c = take_fd(s0);
	return err;
}