From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Don't take solock() in soreceive() for SOCK_RAW inet sockets
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 11 Apr 2024 14:42:03 +0200

On Wed, Apr 10, 2024 at 04:55:49PM +0300, Vitaliy Makkoveev wrote:
> Updated to be on top of the tree. Against previous, the shared solock()
> used around sorflush_locked(). Only unix(4) sockets set (*dom_dispose)()
> handler and it actually doesn't need any protection.

This diff passed regress with witness on i386.

OK bluhm@

Some remarks

>  restart:
> -	if ((error = sblock(so, &so->so_rcv, SBLOCKWAIT(flags))) != 0) {
> -		sounlock_shared(so);
> -		return (error);
> -	}
> +	if ((error = sblock(so, &so->so_rcv, SBLOCKWAIT(flags))) != 0)
> +		goto out;
>  	sb_mtx_lock(&so->so_rcv);

I would prefer instead of goto out

	if (...) {
		if (dosolock)
			sounlock_shared(so);
		return (error);
	}

Every return block needs its own unlock code and scolling
to the labals at end makes it harder to understand.

> +			if (dosolock) {
> +				sb_mtx_unlock(&so->so_rcv);
> +				error = sbwait(so, &so->so_rcv);
> +				if (error) {
> +					sbunlock(so, &so->so_rcv);
> +					sounlock_shared(so);
> +					return (0);
> +				}
> +				sb_mtx_lock(&so->so_rcv);
> +			} else {
> +				if (sbwait_locked(so, &so->so_rcv)) {
> +					error = 0;
> +					goto release;
> +				}
>  			}

Here also I would prefer to have similar code for both blocks
with explicit unlocking.

			if (dosolock) {
				sb_mtx_unlock(&so->so_rcv);
				error = sbwait(so, &so->so_rcv);
				if (error) {
					sbunlock(so, &so->so_rcv);
					sounlock_shared(so);
					return (0);
				}
				sb_mtx_lock(&so->so_rcv);
			} else {
				error = sbwait_locked(so, &so->so_rcv);
				if (error) {
					sb_mtx_unlock(&so->so_rcv);
					sbunlock(so, &so->so_rcv);
					return (0);
				}
			}

Then the difference is clearer.

As the diff has already been tested, you can also commit your version.

bluhm