fallback to soreceive_generic in soreceive_stream for cases not handled correctly currently

[mmacy]

Summary

I have a patch to soreceive_stream to eliminate socket buffer contention, but before upstreaming that I need to ensure that I can actually enable soreceive_stream by default on TCP connections (it's currently an off by default tunable). The fact that soreceive_stream didn't actually work for X forwarding prior to my change this morning leads me to believe that this code hasn't been used much at all. Thus this should also be a request for more general review of soreceive_stream itself.

soreceive_dgram will:

	/*
	 * For any complicated cases, fall back to the full
	 * soreceive_generic().
	 */
	if (mp0 != NULL || (flags & MSG_PEEK) || (flags & MSG_OOB))
		return (soreceive_generic(so, psa, uio, mp0, controlp,
		    flagsp));

This changes soreceive_stream to do the same. It also allows us to simplify the fast path code that checked for MSG_WAITALL and MSG_PEEK.

Test Plan

Production canary at LLNW.

[mmacy]

• remove missed case

[markj]

sys/kern/uipc_socket.c

2184

I think this part of your original change is incomplete, since we now we just silently ignore control messages in the stream. Should we instead call soreceive_generic() if we're expecting to receive a control message?

Why is Xorg expecting to receive a control message on a TCP socket?

2185

You can include MSG_OOB in this check and save a conditional branch.

[mmacy]

sys/kern/uipc_socket.c

2184

I think this part of your original change is incomplete, since we now we just silently ignore control messages in the stream. Should we instead call soreceive_generic() if we're expecting to receive a control message?

I only saw it being used for MSG_PEEK in generic. However, I'll double check.

Why is Xorg expecting to receive a control message on a TCP socket?

It doesn't make any sense to me to but it's definitely getting passed a controlp pointer, but flags are set to zero, so there's nothing actually to be done with it.

[kbowling]

The general consensus on the last TCP call was to commit this. We are not likely to see any issues on LLNW or OCA workload so need the help of the broader community to help stabilize or revert it before 12.0.

[kbowling]

sys/kern/uipc_socket.c

2184

I don't really understand what the line in question is accomplishing but it matches soreceive_generic. It seems like the start of a pointer chase to get the control message into the current 'm' variable for processing. I think the little nugget from soreceive_dgram could be copied up here "for any complicated cases, fall back to the full soreceive_generic"

[markj]

In D15765#345476, @kbowling wrote:

The general consensus on the last TCP call was to commit this. We are not likely to see any issues on LLNW or OCA workload so need the help of the broader community to help stabilize or revert it before 12.0.

This is kernel code whose flow can be directed by untrusted userland code. It should be reviewed carefully.

I don't think my feedback would take more than 15 minutes to address. All I'm pointing out is that:

1. If the caller is prepared to handle a control message, we should probably instead be using soreceive_generic().
2. You can micro-optimize by calling soreceive_generic() when MSG_PEEK is set.

[kbowling]

In D15765#345582, @markj wrote:

In D15765#345476, @kbowling wrote:

The general consensus on the last TCP call was to commit this. We are not likely to see any issues on LLNW or OCA workload so need the help of the broader community to help stabilize or revert it before 12.0.

This is kernel code whose flow can be directed by untrusted userland code. It should be reviewed carefully.

Ouch, not sure where you're coming from here. I politely suggest that if you suspect something like that you need to back it up even if speculative with a control flow, overflow, leak, or UB error. The code was eyeballed by the transport working group https://wiki.freebsd.org/TransportProtocols/28Jun2018 to include Tuexen, JTL, and others, been open publicly for weeks, and last but certainly not least Matt's bona fides.

I don't think my feedback would take more than 15 minutes to address. All I'm pointing out is that:

1. If the caller is prepared to handle a control message, we should probably instead be using soreceive_generic().

Can you clarify what you mean? I've inspected the code on fresh brain and coffee and it looks like MSG_PEEK and MSG_WAITALL are passed off to soreceive_generic code, and MSG_OOB is directly handled correctly. It looks like MSG_WAITALL/DONTWAIT/NBIO are also handled correctly.

I do see one potential issue, pr->pr_flags may need to be checked for PR_ATOMIC and also fall through to so_receivegeneric. There is a test in tools/regression/sockets/pr_atomic/pr_atomic.c that can be extended to use PF_INET/SOCK_STREAM.

2. You can micro-optimize by calling soreceive_generic() when MSG_PEEK is set.

[markj]

In D15765#345637, @kbowling wrote:

In D15765#345582, @markj wrote:

In D15765#345476, @kbowling wrote:

The general consensus on the last TCP call was to commit this. We are not likely to see any issues on LLNW or OCA workload so need the help of the broader community to help stabilize or revert it before 12.0.

This is kernel code whose flow can be directed by untrusted userland code. It should be reviewed carefully.

Ouch, not sure where you're coming from here.

1. review was created
2. I made comments on the review
3. comments weren't addressed
4. "just commit the change"

I politely suggest that if you suspect something like that you need to back it up even if speculative with a control flow, overflow, leak, or UB error.

I don't suspect anything in particular. I'm making the point that changes to this code area should be reviewed. If someone else already has, great, ship it. But there's no indication of that here.

The code was eyeballed by the transport working group https://wiki.freebsd.org/TransportProtocols/28Jun2018 to include Tuexen, JTL, and others, been open publicly for weeks, and last but certainly not least Matt's bona fides.

No one else is making comments on the review, so how am I supposed to know that anyone else has looked at the change? Its existence doesn't mean anything.

I don't think my feedback would take more than 15 minutes to address. All I'm pointing out is that:

1. If the caller is prepared to handle a control message, we should probably instead be using soreceive_generic().

Can you clarify what you mean? I've inspected the code on fresh brain and coffee and it looks like MSG_PEEK and MSG_WAITALL are passed off to soreceive_generic code, and MSG_OOB is directly handled correctly. It looks like MSG_WAITALL/DONTWAIT/NBIO are also handled correctly.

controlp may be non-NULL independent of whether MSG_PEEK or MSG_WAITALL is set. The reason the old code returned EINVAL when controlp is non-NULL is because it doesn't attempt handle that case (even though there's currently no way it can happen). I'm saying that rather than silently ignoring a non-NULL controlp here (i.e., masking some strange behaviour by the caller, such as calling recvmsg() on a TCP socket) we should maybe at least fall back to the generic code.