From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Unlock udp(4) somove()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 19 Jul 2024 20:32:23 +0200

On Fri, Jul 19, 2024 at 09:21:15PM +0300, Vitaliy Makkoveev wrote:
> > On 19 Jul 2024, at 20:50, Alexander Bluhm <alexander.bluhm@gmx.net> wrote:
> > 
> > On Fri, Jul 19, 2024 at 07:21:40PM +0300, Vitaliy Makkoveev wrote:
> >> I don't know is EPIPE correct here. What about to check `so_pcb' and set
> >> error to EPIPE in somove?
> >> 
> >>        if (error) {
> >>                if (sosp->so_snd.sb_state & SS_CANTSENDMORE ||
> >> 		    sosp->so_pcb == NULL)
> >>                        error = EPIPE;
> >>                goto release;
> >>        }
> > 
> > Good idea.
> > 
> >>  nextpkt:
> >> -	if (so->so_error) {
> >> -		error = so->so_error;
> >> +	mtx_enter(&so->so_rcv.sb_mtx);
> >> +	mtx_enter(&sosp->so_snd.sb_mtx);
> > 
> > Can we move locking mutex before nextpkt?
> > 
> >> 	if (m == NULL) {
> >> +		mtx_leave(&sosp->so_snd.sb_mtx);
> >> 		sbdroprecord(so, &so->so_rcv);
> >> +		mtx_leave(&so->so_rcv.sb_mtx);
> >> 		if (so->so_proto->pr_flags & PR_WANTRCVD)
> >> 			pru_rcvd(so);
> >> 		goto nextpkt;
> > 
> > Then we don't have to unlock here.
> 
> Are we 100% sure in tcp_rcvd() and the future tcp_rcvd()? We also call
> it from soreceive() path where we release mutex first.

Look at my proposal.  I did not change unlocking of pru_rcvd().  I
just moved it directly around unlock/pru_rcvd/lock.  Only difference
is that sbdroprecord() runs with sosp->so_snd.sb_mtx locked.  That
does not matter.  In the UDP path we do not unlock/lock when we go
to nextpkt.

> Anyway I want to keep pru_rcvd() calls consistent. Both uipc_rcvd() and
> route_rcvd() don???t have sleep points, so it looks like we could avoid
> mutexes re-locking, but uipc_rcvd() takes both mutexes on so_rcv and
> so_snd. So there is hypothetical problem with somove() and unix(4)
> sockets.

I think this is a misunderstanding.  I changed 3 blocks around
nextpkt.  Only the final one removes all unlock.

bluhm