From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: nfs shared socket lock
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Sat, 15 Feb 2025 01:39:23 +0300

> On 14 Feb 2025, at 22:02, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> Hi,
> 
> The socket functions that are called by NFS code are MP safe.
> Shared net lock together with socket lock is sufficient.
> 
> ok?
> 

I pointed two hunks inline, where socket lock could be omitted.

> bluhm
> 
> Index: nfs/krpc_subr.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/krpc_subr.c,v
> diff -u -p -r1.39 krpc_subr.c
> --- nfs/krpc_subr.c	1 May 2024 13:15:59 -0000	1.39
> +++ nfs/krpc_subr.c	13 Feb 2025 12:54:08 -0000
> @@ -276,9 +276,9 @@ krpc_call(struct sockaddr_in *sa, u_int 
> 	sin->sin_family = AF_INET;
> 	sin->sin_addr.s_addr = INADDR_ANY;
> 	sin->sin_port = htons(0);
> -	solock(so);
> +	solock_shared(so);
> 	error = sobind(so, m, &proc0);
> -	sounlock(so);
> +	sounlock_shared(so);
> 	m_freem(m);
> 	if (error) {
> 		printf("bind failed\n");
> Index: nfs/nfs_socket.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_socket.c,v
> diff -u -p -r1.155 nfs_socket.c
> --- nfs/nfs_socket.c	30 Jan 2025 14:40:50 -0000	1.155
> +++ nfs/nfs_socket.c	13 Feb 2025 12:48:34 -0000
> @@ -281,9 +281,9 @@ nfs_connect(struct nfsmount *nmp, struct
> 		sin->sin_family = AF_INET;
> 		sin->sin_addr.s_addr = INADDR_ANY;
> 		sin->sin_port = htons(0);
> -		solock(so);
> +		solock_shared(so);
> 		error = sobind(so, nam, &proc0);
> -		sounlock(so);
> +		sounlock_shared(so);
> 		if (error)
> 			goto bad;
> 
> @@ -305,7 +305,7 @@ nfs_connect(struct nfsmount *nmp, struct
> 			goto bad;
> 		}
> 	} else {
> -		solock(so);
> +		solock_shared(so);
> 		error = soconnect(so, nmp->nm_nam);
> 		if (error)
> 			goto bad_locked;
> @@ -330,7 +330,7 @@ nfs_connect(struct nfsmount *nmp, struct
> 			so->so_error = 0;
> 			goto bad_locked;
> 		}
> -		sounlock(so);
> +		sounlock_shared(so);
> 	}
> 	/*
> 	 * Always set receive timeout to detect server crash and reconnect.
> @@ -367,7 +367,7 @@ nfs_connect(struct nfsmount *nmp, struct
> 	} else {
> 		panic("%s: nm_sotype %d", __func__, nmp->nm_sotype);
> 	}
> -	solock(so);
> +	solock_shared(so);
> 	error = soreserve(so, sndreserve, rcvreserve);
> 	if (error)
> 		goto bad_locked;
> @@ -377,7 +377,7 @@ nfs_connect(struct nfsmount *nmp, struct
> 	mtx_enter(&so->so_snd.sb_mtx);
> 	so->so_snd.sb_flags |= SB_NOINTR;
> 	mtx_leave(&so->so_snd.sb_mtx);
> -	sounlock(so);
> +	sounlock_shared(so);
> 

The socket lock is not required for sb_flags modification. I left
it to avoid SB_MTXLOCK dances, but now it could be completely
removed.

You could follow the FIOASYNC case of soo_ioctl() and take both
mutexes together if you want atomicy.

> 	m_freem(mopt);
> 	m_freem(nam);
> @@ -390,7 +390,7 @@ nfs_connect(struct nfsmount *nmp, struct
> 	return (0);
> 
> bad_locked:
> -	sounlock(so);
> +	sounlock_shared(so);
> bad:
> 
> 	m_freem(mopt);
> Index: nfs/nfs_syscalls.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_syscalls.c,v
> diff -u -p -r1.128 nfs_syscalls.c
> --- nfs/nfs_syscalls.c	18 Sep 2024 05:21:19 -0000	1.128
> +++ nfs/nfs_syscalls.c	13 Feb 2025 12:55:27 -0000
> @@ -247,9 +247,9 @@ nfssvc_addsock(struct file *fp, struct m
> 		siz = NFS_MAXPACKET + sizeof (u_long);
> 	else
> 		siz = NFS_MAXPACKET;
> -	solock(so);
> +	solock_shared(so);
> 	error = soreserve(so, siz, siz); 
> -	sounlock(so);
> +	sounlock_shared(so);
> 	if (error) {
> 		m_freem(mynam);
> 		return (error);
> @@ -275,7 +275,7 @@ nfssvc_addsock(struct file *fp, struct m
> 		sosetopt(so, IPPROTO_TCP, TCP_NODELAY, m);
> 		m_freem(m);
> 	}
> -	solock(so);
> +	solock_shared(so);
> 	mtx_enter(&so->so_rcv.sb_mtx);
> 	so->so_rcv.sb_flags &= ~SB_NOINTR;
> 	so->so_rcv.sb_timeo_nsecs = INFSLP;
> @@ -284,7 +284,7 @@ nfssvc_addsock(struct file *fp, struct m
> 	so->so_snd.sb_flags &= ~SB_NOINTR;
> 	so->so_snd.sb_timeo_nsecs = INFSLP;
> 	mtx_leave(&so->so_snd.sb_mtx);
> -	sounlock(so);
> +	sounlock_shared(so);
> 	if (tslp)
> 		slp = tslp;
> 	else {
> 

The same as above, socket lock is not required here.