From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: socket splicing oob inline race
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Wed, 22 Jan 2025 17:53:11 +0300

On Wed, Jan 22, 2025 at 02:59:11PM +0100, Alexander Bluhm wrote:
> Hi,
> 
> Socket slicing test regress/sys/kern/sosplice/tcp run-args-oobinline.pl
> failed from time to time.  It is splicing out-of-bound data that
> is inline within the TCP stream.  The test reported wrong offsets
> in the TCP urgent pointer.
> 
> Problem is that length of receive buffer and so_oobmark are not
> modified in the same critical section.  To call pru_rcvd() the
> receive socket buffer is released.  Moving mutex leave and calling
> pru_rcvd() after updating so_oobmark fixes the test.
> 
> When somove() was holding exclusive net lock, order did not matter.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> diff -u -p -U5 -r1.362 uipc_socket.c
> --- kern/uipc_socket.c	21 Jan 2025 17:41:39 -0000	1.362
> +++ kern/uipc_socket.c	22 Jan 2025 13:35:20 -0000
> @@ -1755,31 +1755,31 @@ somove(struct socket *so, int wait)
>  	if (m->m_flags & M_PKTHDR) {
>  		m_resethdr(m);
>  		m->m_pkthdr.len = len;
>  	}
>  
> -	/* Send window update to source peer as receive buffer has changed. */
> -	if (so->so_proto->pr_flags & PR_WANTRCVD) {
> -		mtx_leave(&sosp->so_snd.sb_mtx);
> -		mtx_leave(&so->so_rcv.sb_mtx);
> -		solock_shared(so);
> -		pru_rcvd(so);
> -		sounlock_shared(so);
> -		mtx_enter(&so->so_rcv.sb_mtx);
> -		mtx_enter(&sosp->so_snd.sb_mtx);
> -	}
> -
>  	/* Receive buffer did shrink by len bytes, adjust oob. */
>  	rcvstate = so->so_rcv.sb_state;
>  	so->so_rcv.sb_state &= ~SS_RCVATMARK;
>  	oobmark = so->so_oobmark;
>  	so->so_oobmark = oobmark > len ? oobmark - len : 0;
>  	if (oobmark) {
>  		if (oobmark == len)
>  			so->so_rcv.sb_state |= SS_RCVATMARK;
>  		if (oobmark >= len)
>  			oobmark = 0;
> +	}
> +
> +	/* Send window update to source peer as receive buffer has changed. */
> +	if (so->so_proto->pr_flags & PR_WANTRCVD) {
> +		mtx_leave(&sosp->so_snd.sb_mtx);
> +		mtx_leave(&so->so_rcv.sb_mtx);
> +		solock_shared(so);
> +		pru_rcvd(so);
> +		sounlock_shared(so);
> +		mtx_enter(&so->so_rcv.sb_mtx);
> +		mtx_enter(&sosp->so_snd.sb_mtx);
>  	}
>  
>  	/*
>  	 * Handle oob data.  If any malloc fails, ignore error.
>  	 * TCP urgent data is not very reliable anyway.
>