From: Vitaliy Makkoveev Subject: Re: Don't take solock() in sosend() and soreceive() paths for unix(4) sockets To: Alexander Bluhm Cc: tech@openbsd.org Date: Fri, 3 May 2024 20:44:25 +0300 On Fri, May 03, 2024 at 05:01:59PM +0200, Alexander Bluhm wrote: > On Fri, May 03, 2024 at 02:10:49AM +0300, Vitaliy Makkoveev wrote: > > On Fri, May 03, 2024 at 12:26:32AM +0200, Alexander Bluhm wrote: > > > On Wed, May 01, 2024 at 01:01:22AM +0300, Vitaliy Makkoveev wrote: > > > > On Tue, Apr 30, 2024 at 11:01:04PM +0300, Vitaliy Makkoveev wrote: > > > > > Use `sb_mtx' mutex(9) and `sb_lock' rwlock(9) to protect `so_snd' and > > > > > `so_rcv' of unix(4) sockets. > > > > > > > > > > The transmission of unix(4) sockets already half-unlocked because > > > > > connected peer is not locked by solock() during sbappend*() call. Since > > > > > the `so_snd' is protected by `sb_mtx' mutex(9) the re-locking is not > > > > > required in uipc_rcvd() too. > > > > > > > > > > SB_OWNLOCK became redundant with SB_MTXLOCK, so remove it. SB_MTXLOCK > > > > > was kept because checks against SB_MTXLOCK within sb*() routines look > > > > > more consistent to me. > > > > > > > > > > Please note, the unlocked peer `so2' of unix(4) can't be disconnected > > > > > while solock() is held on `so'. That's why unlocked sorwakeup() and > > > > > sowwakeup() are fine, corresponding paths will never be followed. > > > > > > > > > > > > > Add forgotten fifofs chunks. Against previous, this diff does direct > > > > `so_rcv' dispose and cleanup in sofree(). This sockets is almost dead > > > > and unlinked from everywhere include spliced peer. So concurrent > > > > sotask() thread will just exit. This required to solve inode and so_rcv > > > > locks ordering. Also this removes re-locking from sofree() for all > > > > sockets. > > > > > > Passed regress with witness. > > > > > > One question, otherwise OK bluhm@ > > > > > > [skip] > > > > > > You change the order of EPIPE and ENOTCONN error checks here. > > > The difference is relevant, EPIPE may signal a SIGPIPE. > > > > > > When writing into a socket that stops working, the error should be > > > EPIPE. If there are two processes, A is writing to B, and B > > > disconnects the socket, then A should get a EPIPE. > > > > > > Do you change the behavior? > > > > > > In sosend() we have a SS_CANTSENDMORE check with EPIPE error. > > > So maybe the uipc_send() errors are irrelevant. > > > > > > > Yes, I change behavior. I had concerns , so I checked FreeBSD version of > > uipc_send() and found that they do "so->so_state & SS_ISCONNECTED" check > > before "so->so_snd.sb_state & SBS_CANTSENDMORE". But sosend_generic() > > has the different order. Since this code exists for a long time, I found > > this order not significant. > > man 2 write > [EPIPE] An attempt is made to write to a socket of type > SOCK_STREAM that is not connected to a peer socket. > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html > [EPIPE] > A write was attempted on a socket that is shut down for > writing, or is no longer connected. In the latter case, if > the socket is of type SOCK_STREAM, a SIGPIPE signal shall > also be sent to the thread. > > In general this is true, sosend_generic() and sosend() check > SS_CANTSENDMORE first within the restart loop. Maybe we can consider > the other case as MP race with wrong error code that should not > happen. > > As other BSD also do it this way, your diff is OK bluhm@ > Thanks! If we found any fallout of this reordering, it is easy to turn order back.