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.