Is this logically related to the rest of the change?

IMO, "reset" is more precise than "process" and so would be a better name.

Why not handle the c != NULL case unconditionally? I understand that m != NULL implies c == NULL today, but there's no obvious reason to assume that. If there's an unobvious reason, there should be a comment.

This can be STAILQ_FOREACH_FROM as well.

I don't understand this - how can you set STAILQ_FIRST of a non-empty queue to control without also updating control?

Can this mostly self-contained operation be moved into a separate subroutine? There is too much indentation here.

We don't need the blank line anymore.

It would be better to add MCHAIN_FOREACH(_SAFE) or so instead of open-coding like this.

It is. Previously this was done by the generic socket code when a socket switches from a true socket with buffers to a listening. This is first protocol that is PR_SOCKBUF and can listen(). See also 7ee47c3bb7a6d85744e2747ec969161eed5bbaab.

To me it seems more logical and easier to read to separate the user write and kernel write into two separated blocks. I did the same with unix/dgram.

Nope :( MSG_EOR and M_EOR are two different things.

Although we drop the lock and the take it back, we are the only thread that may drain the queue. Exclusion is shutdown(2), but it is covered above. That means that the very last mbuf in the chain we were planning for copyout, but now we are gluing back due to failure, still has m_stailq pointing to the very first mbuf in the queue.

This can be STAILQ_FOREACH_FROM as well.

Unfortunately no. Entering with first == NULL the existing cycle will not be executed, but STAILQ_FOREACH_FROM() will start from the beginning.

Call it uipc_stream_sbspace(), for consistency with uipc_dgram_sbspace()?

This might unlock unp. Do we need to check so->so_error != 0 again?

The locking protocol for so_error is not too clear to me. Some code uses the PCB lock to serialize updates, some uses the socket lock.

Don't we want to hold the unp lock while reading the state?

There are more instances of this below.

Hmm, if mc is empty, then after copying this way the tail pointer of mcnext will be incorrect. Does that matter? It seems like a subtle source of bugs.

So why do we add the virtual send buffer size?

The access of sb_hiwat is unlocked. If this is intentional, it should be atomic_load_int(&so->so_snd.sb_hiwat).

Don't we want to check this with the PCB or socket lock held?

Here we are removing a chain of mbufs from the socket buffer. But, isn't the last mbuf in the chain still pointing to the first mbuf still in the buffer? I cannot see where the m_next field of the last mbuf is set to NULL.

Below there is an error path where we m_freem(control), but then it looks like it might free the mbufs still lingering in the socket buffer.

Thanks, I will rewrite these two checks to check so_error and so_state after acquiring the second unp lock.

The locking protocol for so_error remains the same as before the change - not very consistent, but working, IMHO. What happens here is that so_error is set either in other syscall on the same socket or in unp_drop(). The latter is protected by unp lock. The race between two syscalls is practically impossible due to socket I/O lock.

This is how any other code operating on STAILQ or TAILQ headers work. It is incorrect to dereference STAILQ_LAST on a queue that is not known to be not empty.

Because now we know that there is space in the receive buffer and our write is guaranteed to commit at least space bytes to receive buffer. Then most likely we will be put to sleep. And we will be sleeping with so->so_snd.sb_hiwat bytes in our hands, which effectively is the send buffer, although it isn't linked to anywhere. Note that very first "fast path" mc_uiotomc() at the beginning of the function is done only with send buffer space size, cause we don't yet know about receive buffer.

I don't agree. If we got two threads chaotically doing two syscalls: setsockopt(SO_SNDBUF) and send() they can race anyway, either in the userland or when locking the socket.

If we are talking about a normal application that sets buffer and only then does sends, it should see the updated value in the send(). Because setsockopt() updated hiwat with socket buffer lock, and then send() acquired the same mutex at least once before it came to this place.

P.S. unix/dgram isn't special here. This applies to all other protocols.

Using unp lock in this function would definitely affect performance. The check is basically against a socket that was never connected at all. So only an application that chaotically does 3 syscalls: socket(2), connect(2), recv(2) would ever see a problem here. A normal application that would do a recv() only after previous connect() returned, wouldn't observe any problem here. In the later lifetime of a socket this check is always false. Even if we do not see the clearing of SS_ISCONNECTED and raising of SS_ISDISCONNECTED by unp_soisdisconnected(), we would still behave correct here. So the race with unp_soisdisconnected() is harmless, and end result is the same - we see SBS_CANTRCVMORE few lines later and return 0.

For reference: the soreceive_generic() does this check under socket receive buffer lock, however it does it after SBS_CANTRCVMORE check. End result for application is the same - ENOTCONN only on a never connected socket. Also, generic code doesn't protect these bits of so_state with socket receive buffer lock. We can do that easily in unp_soisdisconnected(), but we don't really need to.

The SS_ISCONNECTED doesn't need locking as it is 0 on a new born socket and is set on connect(). The SS_ISDISCONNECTED is set in unp_soisdisconnected(). If we miss update of the flag here than we will see SBS_CANTRCVMORE later. So nothing critical is going to happen except we return 0 instead of ENOTCONN. Potentially I can move this check down into the cycle and check with receive buffer lock. This is actually what soreceive_generic() does! But, soreceive_generic() checks that after SBS_CANTRCVMORE check, so it will also return 0 instead of ENOTCONN in case of a race.

Thanks, that's a good find! As I was working on a fix, I realized that the code that covers race with shutdown(2) and does m_freem(control) was added here after I did an incorrect 507f87a799cf0811ce30f0ae7f10ba19b2fd3db3 and before I reverted it. Now this race can't happen and the error handling code disappears.

I'm a bit worried that this is too expensive. Did you try the patch on a desktop system?

It might be worth adding a sysctl to disable this checking.

Could I ask you to explain this in a comment? IMO the one that is there is too terse.

My suggestion is just to acknowledge the existence of races here by using atomic_load_int(). It is not required for correctness, but it IMO makes the code easier to understand and also helps annotate "harmless" races for concurrency sanitizers.

I think some summary of this explanation should be added as a comment, and the check should be written:

if (__predict_false((atomic_load_short(&so->so_state) &
	    (SS_ISCONNECTED|SS_ISDISCONNECTED)) == 0))

It's too difficult for readers to understand why these kinds of races are safe, so IMHO we should try harder to annotate them in new code.

Oh, that's weird. Isn't this also simply incorrect, given that the sizes of an internalized message and its externalized form are not always the same?

You could write this loop like this to make it simpler:

error = 0;
for (m = first; m != last; m = next) {
    next = STAILQ_NEXT(m, m_stailq);
    if (__predict_true(error == 0))
        error = uiomove(mtod(m, char *), m->m_len, uio);
    if (!peek)
        m_free(m);
}
if (__predict_false(error != 0)) {
    SOCK_IO_RECV_UNLOCK(so);
    return (error);
}

This way, you hold on to the lock in the error case, but I think that's ok.

Can this be written last->m_data - M_START(last) instead?

Right now typing on a system running with the patch and INVARIANTS. But that's Threadripper PRO. But I agree. I will just move this under SOCKBUF_DEBUG, cause it does exactly same function.

There is a wordy comment on that above the declaration of socket buffer space sysctls. I can refer from here to up there.

I can't agree with that. Adding the atomic is not just a documentation, it affects performance. It also doesn't look like a correct way to fix the race if the race wasn't harmless. It may help to shut up sanitizers, but it will also mislead a new human reader of the code. The existence of atomic would make them think that a) there is a race here that must be addressed b) it is addressed in the right way. I'd call that anti-documentation.

I will provide a comment.

Absolutely! Let's fix this after.

I did that, looked at and returned back. May I leave as is? IMHO, a human reader prefers to see in the code the streamlined non-error fast path with stub error branches. A cycle that covers both requires certain mind wrap. I guess compiler would make it his way in either case.

I has always assumed that any atomic(9) adds a lock prefix and I was wrong. I tried compiling with atomic and difference was minimal, before:

42eb: 03 90 ac 02 00 00             addl    0x2ac(%rax), %edx

after:

42eb: 8b 80 ac 02 00 00             movl    0x2ac(%rax), %eax
42f1: 01 c2                         addl    %eax, %edx

I will add atomic_load_int() here and atomic_load_short() where we check (SS_ISCONNECTED|SS_ISDISCONNECTED).