hastd: fix fd passing over socketpair

[xtronom_gmail.com]

Summary

The connection migration path transfers a protocol name and a connected
socket descriptor over a local socketpair.

The protocol name is currently transmitted using send(2), while the
descriptor is transmitted using a separate sendmsg(2) call carrying
SCM_RIGHTS ancillary data. The receiver expects both pieces of information
to arrive together and uses a receive buffer sized for the maximum
protocol name length.

On FreeBSD 15 this can cause the receive side to block indefinitely,
preventing the migrated connection from being delivered to the worker
process. As a result, the primary node never sends the HAST protocol
header and the secondary node times out waiting for it.

Transmit the protocol name and descriptor together using a single
sendmsg(2) operation and receive them using a single recvmsg(2)
operation.

Test Plan

On two FreeBSD 15 systems configured as a HAST pair:

1. Start hastd on both nodes.
2. Set the resource role to secondary on one node.
3. Set the resource role to primary on the other node.
4. Verify that /dev/hast/<resource> is created on the primary.
5. Verify that geom gate list and ggatel list no longer block on primary.
6. Verify that the secondary no longer logs "Unable to receive header".
7. Verify that hastctl status reports the resource connected and synchronized.

[glebius]

I really can't understand how this worked before FreeBSD 15. If one side sends less than 128 bytes, but other side says recv(s, buf, 128, MSG_WAITALL), then the other side shall hang always. I don't remember that my changes to unix(4) in FreeBSD 15 had fixed ignored MSG_WAITALL. Was that some other data sent after that woke up the receiver?

Anyway, thanks a lot for diagnosing the problem. The fix looks correct, but not beautiful. I would refactor out those proto_common_*. IMHO, the proto_socketpair and proto_uds should just use the proto_descriptor_send() and proto_descriptor_recv() that are capable to send data and descriptor or just or just data. These two functions can be renamed to proto_unix_send and proto_unix_recv and the proto_common left for TCP.

Let's hear what Pawel thinks.

[xtronom_gmail.com]

In D57511#1318085, @glebius wrote:

I really can't understand how this worked before FreeBSD 15. If one side sends less than 128 bytes, but other side says recv(s, buf, 128, MSG_WAITALL), then the other side shall hang always. I don't remember that my changes to unix(4) in FreeBSD 15 had fixed ignored MSG_WAITALL. Was that some other data sent after that woke up the receiver?

I wrote a small socketpair test matching the HAST sequence:

send("tcp\0", 4)
sendmsg(SCM_RIGHTS only, no iov)
recv(..., 127, MSG_WAITALL)
recvmsg(...)

On FreeBSD 13 the second sendmsg() wakes the MSG_WAITALL recv, which returns short with the 4-byte protocol name, and the following recvmsg() receives the fd.

parent: send('tcp\0') = 4 errno=0
parent: sendmsg(fd-only) = 0 errno=0
child: recv(MSG_WAITALL, 127) = 4 errno=0 buf='tcp'
child: recvmsg(fd-only) = 0 errno=0 flags=0x0
child: received fd=3

On FreeBSD 15 the MSG_WAITALL recv remains blocked.

parent: send('tcp\0') = 4 errno=0
parent: sendmsg(fd-only) = 0 errno=0

One could argue that either behavior is correct. FreeBSD 13 wakes the waiting recv(2) and returns a short read when the ancillary data arrives, while FreeBSD 15 continues waiting (no signal, recv() not interrupted) for the requested byte count.

However, the parent/worker communication channel is a SOCK_STREAM socketpair. The descriptor transfer is performed using sendmsg(2) with SCM_RIGHTS and no payload data.

The proposed change avoids depending on this interaction by transmitting the protocol name and descriptor as a single logical message.

Anyway, thanks a lot for diagnosing the problem. The fix looks correct, but not beautiful. I would refactor out those proto_common_*. IMHO, the proto_socketpair and proto_uds should just use the proto_descriptor_send() and proto_descriptor_recv() that are capable to send data and descriptor or just or just data. These two functions can be renamed to proto_unix_send and proto_unix_recv and the proto_common left for TCP.

The proto layer supports three transports and there is no clean way to use transport-specific helpers directly once the abstraction layer is in use. The proto_common_send()/proto_common_recv() helpers also carry several overloaded semantics:

Transport      Used for              Exact I/O   NULL direction       NULL shutdown   FD passing
tcp            replication           yes         not supported        unused          prohibited
uds            hastctl control       yes         not supported        unused          not used
socketpair     parent/worker IPC     yes         yes (after fork)     not possible    yes

The failing path is the socketpair parent/worker connection migration path. A cleaner refactor would likely separate exact-size stream I/O from UNIX descriptor-passing I/O and reduce the overloaded semantics in the common helpers.

However, doing so would require broader changes to the transport abstraction and its implementations. This revision keeps the change limited to the failing descriptor-passing path and avoids modifying the rest of the protocol layer.

Let's hear what Pawel thinks.

[glebius]

In D57511#1318143, @xtronom_gmail.com wrote:

On FreeBSD 13 the second sendmsg() wakes the MSG_WAITALL recv, which returns short with the 4-byte protocol name, and the following recvmsg() receives the fd.

Hmm, looks like new unix(4) had fixed a bug. The bug was kinda documented in recv(2):

The MSG_WAITALL flag requests that the operation block until the full request is satisfied. However, the call may still return less data than requested if a signal is caught, an error or disconnect occurs, or the next data to be received is of a different type than that returned.

The page doesn't explain what is data type, but we could guess that control message is referred to.

However SUS doesn't say that:

MSG_WAITALL On SOCK_STREAM sockets this requests that the function block until the full amount of data can be returned. The function may return the smaller amount of data if the socket is a message-based socket, if a signal is caught, if the connection is terminated, if MSG_PEEK was specified, or if an error is pending for the socket.

I don't think I should restore the old behavior, better stick to the behavior specified by the standard.

[glebius]

The proto layer supports three transports and there is no clean way to use transport-specific helpers directly once the abstraction layer is in use. The proto_common_send()/proto_common_recv() helpers also carry several overloaded semantics:
The failing path is the socketpair parent/worker connection migration path. A cleaner refactor would likely separate exact-size stream I/O from UNIX descriptor-passing I/O and reduce the overloaded semantics in the common helpers.
However, doing so would require broader changes to the transport abstraction and its implementations. This revision keeps the change limited to the failing descriptor-passing path and avoids modifying the rest of the protocol layer.

That's why I propose the remove this abstraction as it creates more trouble and obfuscation then benefit, IMHO. What's the point of abstraction if certain protocol calls are immediately short-circuited to a different function and the main sending (or receiving) loop is not entered at all?

Ok, let's hear from Pawel.

[xtronom_gmail.com]

In D57511#1318443, @glebius wrote:

That's why I propose the remove this abstraction as it creates more trouble and obfuscation then benefit, IMHO. What's the point of abstraction if certain protocol calls are immediately short-circuited to a different function and the main sending (or receiving) loop is not entered at all?

Ok, let's hear from Pawel.

I agree with the direction. I'd like to hear Pawel's thoughts before reworking the abstraction, since he may have context on why it was structured this way originally.

[glebius]

I added Dag-Erling as he recently committed to hastd.

xtronom, looks like we won't hear from Pawel. May I ask you to implement what we discussed earlier without waiting for Pawel, and then me and Dag-Erling will review and push.

[des]

There might be a solution that doesn't require changing the internal API: don't use MSG_WAITALL. You will however have to think carefully about short read semantics.

[glebius]

In D57511#1362362, @des wrote:

There might be a solution that doesn't require changing the internal API: don't use MSG_WAITALL. You will however have to think carefully about short read semantics.

Up to your decision, guys. IMHO, an contract within a single file is not an API to care about. MSG_WAITALL really makes things easier for application writer.

@xtronom_gmail.com are you interested in refactoring this revision in either direction? Assuming Dag-Erling is willing to push it.

[des]

In D57511#1363508, @glebius wrote:

In D57511#1362362, @des wrote:

There might be a solution that doesn't require changing the internal API: don't use MSG_WAITALL. You will however have to think carefully about short read semantics.

[...] MSG_WAITALL really makes things easier for application writer.

If short reads are not expected at the application level, it really isn't hard to loop until we've reached the expected length or an error occurs.

[des]

Hang on, I just realized that the scenario described here is one where the receiver expects to read 128 bytes but the sender sends fewer than 128, and recv() is interrupted by the arrival of the descriptor. So we have a short read either way, but no way to inform the caller, who probably does not expect it. That needs to be addressed: proto_common_recv() needs to not use MSG_WAITALL and return or pass the actual amount read to the caller, and every caller needs to check the amount they actually received. This does however mean that most of this patch is unnecessary.

@xtronom_gmail.com do you have a reproducer for this bug?

[xtronom_gmail.com]

Sorry for the delayed response; I was traveling and the discussion moved faster than I could follow.

I'm interested in implementing whichever direction we agree on.

First, let me clarify the original issue and the change in MSG_WAITALL behavior which exposed it.

With the older AF_UNIX implementation, the following sequence worked:

send("tcp\0", 4)
sendmsg(SCM_RIGHTS)

recv(buf, 127, MSG_WAITALL) -> returns 4
recvmsg(...)                -> receives the descriptor

That is, arrival of the ancillary-data-bearing message caused the MSG_WAITALL receive to return short. The existing HAST code relies on that behavior: after the short read it proceeds to recvmsg() to receive the descriptor.

On FreeBSD 15.x and CURRENT, arrival of the descriptor no longer causes the recv(..., MSG_WAITALL) call to return. It continues waiting for the remaining bytes. Consequently, the worker never reaches recvmsg(), never receives the migrated connection from the parent, and therefore never sends the HAST protocol header to the secondary.

Here is a standalone reproducer:

#include <sys/types.h>
#include <sys/socket.h>
#include <sys/uio.h>
#include <sys/wait.h>

#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

static void
send_fd_only(int sock, int fd)
{
	char ctrl[CMSG_SPACE(sizeof(fd))];
	struct msghdr msg = {0};
	struct cmsghdr *cmsg;

	memset(ctrl, 0, sizeof(ctrl));
	msg.msg_control = ctrl;
	msg.msg_controllen = sizeof(ctrl);

	cmsg = CMSG_FIRSTHDR(&msg);
	cmsg->cmsg_level = SOL_SOCKET;
	cmsg->cmsg_type = SCM_RIGHTS;
	cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
	memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));

	ssize_t n = sendmsg(sock, &msg, 0);
	printf("parent: sendmsg(fd-only) = %zd errno=%d\n", n, errno);
}

static void
recv_fd_only(int sock)
{
	char ctrl[CMSG_SPACE(sizeof(int))];
	struct msghdr msg = {0};
	struct cmsghdr *cmsg;
	int fd = -1;

	memset(ctrl, 0, sizeof(ctrl));
	msg.msg_control = ctrl;
	msg.msg_controllen = sizeof(ctrl);

	ssize_t n = recvmsg(sock, &msg, 0);
	printf("child: recvmsg(fd-only) = %zd errno=%d flags=0x%x\n",
	    n, errno, msg.msg_flags);

	cmsg = CMSG_FIRSTHDR(&msg);
	if (cmsg != NULL && cmsg->cmsg_level == SOL_SOCKET &&
	    cmsg->cmsg_type == SCM_RIGHTS) {
		memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
		printf("child: received fd=%d\n", fd);
		close(fd);
	}
}

int
main(void)
{
	int sv[2];
	pid_t pid;

	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1)
		err(1, "socketpair");

	pid = fork();
	if (pid == -1)
		err(1, "fork");

	if (pid == 0) {
		char buf[128];

		close(sv[0]);

		alarm(5);

		memset(buf, 0, sizeof(buf));
		ssize_t n = recv(sv[1], buf, 127, MSG_WAITALL);
		printf("child: recv(MSG_WAITALL, 127) = %zd errno=%d buf='%s'\n",
		    n, errno, buf);

		recv_fd_only(sv[1]);

		_exit(0);
	}

	close(sv[1]);

	usleep(200000);
	ssize_t n = send(sv[0], "tcp", 4, 0);   /* includes NUL */
	printf("parent: send('tcp\\0') = %zd errno=%d\n", n, errno);

	usleep(200000);
	send_fd_only(sv[0], STDOUT_FILENO);

	waitpid(pid, NULL, 0);
	return (0);
}

The idea behind my original patch was to keep the change deliberately narrow:

• preserve the existing replication I/O semantics;
• avoid requiring a broader protocol audit;
• minimize the risk of changing behavior that could affect live data;
• avoid implementing buffering or pushback in user space where sendmsg() / recvmsg() already provide the appropriate UNIX-domain-socket mechanism;

I still think the I/O-layer refactoring discussed earlier with glebius makes sense. The replication connection, hastctl connection, and parent/worker socketpair have different semantics, and separating them would let each transport use the appropriate primitives instead of forcing them through proto_common_recv() / proto_common_send().

For the parent/worker socketpair, I think a single sendmsg() / recvmsg() exchange containing both the protocol name and SCM_RIGHTS descriptor is the natural interface. It also removes the dependency on the old short-read behavior entirely.

For the replication path, the situation is different. The protocol has explicit framing: a fixed-size header describes the payload size, followed by a payload of that known size. There, replacing MSG_WAITALL with an explicit exact-read loop is straightforward in principle.

However, simply removing MSG_WAITALL and looping until size bytes have been received does not fix the original parent/worker problem. In that path, size is the capacity of the protocol-name buffer, not the length of a framed message. An exact-read loop would therefore reproduce the same wait in user space: it would receive "tcp\0" and then wait for the remaining bytes while the descriptor is waiting to be collected by recvmsg().

So my preference would be to separate the transport semantics first: use sendmsg() / recvmsg() for the descriptor-passing UNIX socket path, and use explicit framed stream reads for replication. At that point, removing MSG_WAITALL from the replication path becomes much easier to reason about and validate.

[des]

In D57511#1363899, @xtronom_gmail.com wrote:

I'm interested in implementing whichever direction we agree on.

I already have a patch.

First, let me clarify the original issue and the change in MSG_WAITALL behavior which exposed it.

You don't need to clarify anything. I already understand.

Here is a standalone reproducer:

I'm not interested in a standalone reproducer. We've already established that the kernel is behaving correctly here. I asked if you had a reproducer for the hastd issue.