Re: [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to_connected()

On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:

On 7/19/24 13:09, Jakub Sitnicki wrote:

quoted

On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:

quoted

On 7/13/24 11:45, Jakub Sitnicki wrote:

quoted

On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:

quoted

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.

CC'ed BPF selftests reviewers in case they'd like to chip in.

quoted

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?

I'm fine with having such a helper to complement the cleanup attribute.
Alternatively, we can always open code it like it used to be in systemd
at first [1], if other reviewers don't warm up to it :-)

[1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci

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

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);

I'd stick to no declarations in the body. Init to -1 or -EBADF.

	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);

I wouldn't touch output arguments until we have succedeed.  Another
local var will be handy.

		if (*p < 0)
			return *p;
		break;
	default:
		FAIL("Unsupported socket type %#x", sotype);
		return -EOPNOTSUPP;
	}

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