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@