Index | Thread | Search

From:
Vitaliy Makkoveev <mvs@openbsd.org>
Subject:
Re: lock sonewconn() for internet sockets
To:
Alexander Bluhm <bluhm@openbsd.org>
Cc:
tech@openbsd.org
Date:
Thu, 16 Jan 2025 16:33:41 +0300

Download raw body.

Thread
On Thu, Jan 16, 2025 at 02:22:49PM +0100, Alexander Bluhm wrote:
> Hi,
> 
> I would like to lock the new socket in sonewconn() also for internet
> family.  This is neeed for TCP input unlocking.  When we have to
> lock two sockets, one needs shared netlock, but the other already
> has it.  As this is the same idea as in doaccept() unify the lock
> functions.
> 
> ok?
> 

ok mvs

> bluhm
> 
> Index: kern/uipc_socket2.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket2.c,v
> diff -u -p -r1.164 uipc_socket2.c
> --- kern/uipc_socket2.c	5 Jan 2025 12:36:48 -0000	1.164
> +++ kern/uipc_socket2.c	16 Jan 2025 13:13:38 -0000
> @@ -186,14 +186,8 @@ struct socket *
>  sonewconn(struct socket *head, int connstatus, int wait)
>  {
>  	struct socket *so;
> -	int persocket = solock_persocket(head);
>  	int soqueue = connstatus ? 1 : 0;
>  
> -	/*
> -	 * XXXSMP as long as `so' and `head' share the same lock, we
> -	 * can call soreserve() and pr_attach() below w/o explicitly
> -	 * locking `so'.
> -	 */
>  	soassertlocked(head);
>  
>  	if (m_pool_used() > 95)
> @@ -218,8 +212,7 @@ sonewconn(struct socket *head, int conns
>  	/*
>  	 * Lock order will be `head' -> `so' while these sockets are linked.
>  	 */
> -	if (persocket)
> -		solock(so);
> +	solock_nonet(so);
>  
>  	/*
>  	 * Inherit watermarks but those may get clamped in low mem situations.
> @@ -252,14 +245,12 @@ sonewconn(struct socket *head, int conns
>  		wakeup(&head->so_timeo);
>  	}
>  
> -	if (persocket)
> -		sounlock(so);
> +	sounlock_nonet(so);
>  
>  	return (so);
>  
>  fail:
> -	if (persocket)
> -		sounlock(so);
> +	sounlock_nonet(so);
>  	sigio_free(&so->so_sigio);
>  	klist_free(&so->so_rcv.sb_klist);
>  	klist_free(&so->so_snd.sb_klist);
> @@ -363,12 +354,21 @@ solock_shared(struct socket *so)
>  	case PF_INET:
>  	case PF_INET6:
>  		NET_LOCK_SHARED();
> -		rw_enter_write(&so->so_lock);
>  		break;
> -	default:
> -		rw_enter_write(&so->so_lock);
> +	}
> +	rw_enter_write(&so->so_lock);
> +}
> +
> +void
> +solock_nonet(struct socket *so)
> +{
> +	switch (so->so_proto->pr_domain->dom_family) {
> +	case PF_INET:
> +	case PF_INET6:
> +		NET_ASSERT_LOCKED();
>  		break;
>  	}
> +	rw_enter_write(&so->so_lock);
>  }
>  
>  int
> @@ -416,16 +416,19 @@ sounlock(struct socket *so)
>  void
>  sounlock_shared(struct socket *so)
>  {
> +	rw_exit_write(&so->so_lock);
>  	switch (so->so_proto->pr_domain->dom_family) {
>  	case PF_INET:
>  	case PF_INET6:
> -		rw_exit_write(&so->so_lock);
>  		NET_UNLOCK_SHARED();
>  		break;
> -	default:
> -		rw_exit_write(&so->so_lock);
> -		break;
>  	}
> +}
> +
> +void
> +sounlock_nonet(struct socket *so)
> +{
> +	rw_exit_write(&so->so_lock);
>  }
>  
>  void
> Index: kern/uipc_syscalls.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_syscalls.c,v
> diff -u -p -r1.221 uipc_syscalls.c
> --- kern/uipc_syscalls.c	5 Jan 2025 11:33:45 -0000	1.221
> +++ kern/uipc_syscalls.c	16 Jan 2025 13:01:34 -0000
> @@ -247,34 +247,6 @@ sys_accept4(struct proc *p, void *v, reg
>  	    SCARG(uap, anamelen), SCARG(uap, flags), retval));
>  }
>  
> -void
> -doaccept_solock(struct socket *so, int take_netlock)
> -{
> -	if (take_netlock) {
> -		switch (so->so_proto->pr_domain->dom_family) {
> -		case PF_INET:
> -		case PF_INET6:
> -			NET_LOCK_SHARED();
> -		}
> -	}
> -
> -	rw_enter_write(&so->so_lock);
> -}
> -
> -void
> -doaccept_sounlock(struct socket *so, int release_netlock)
> -{
> -	rw_exit_write(&so->so_lock);
> -
> -	if (release_netlock) {
> -		switch (so->so_proto->pr_domain->dom_family) {
> -		case PF_INET:
> -		case PF_INET6:
> -			NET_UNLOCK_SHARED();
> -		}
> -	}
> -}
> -
>  int
>  doaccept(struct proc *p, int sock, struct sockaddr *name, socklen_t *anamelen,
>      int flags, register_t *retval)
> @@ -307,7 +279,7 @@ doaccept(struct proc *p, int sock, struc
>  	nam = m_get(M_WAIT, MT_SONAME);
>  
>  	head = headfp->f_data;
> -	doaccept_solock(head, 1);
> +	solock_shared(head);
>  
>  	if (isdnssocket(head) || (head->so_options & SO_ACCEPTCONN) == 0) {
>  		error = EINVAL;
> @@ -341,7 +313,7 @@ doaccept(struct proc *p, int sock, struc
>  	 */
>  	so = TAILQ_FIRST(&head->so_q);
>  
> -	doaccept_solock(so, 0);
> +	solock_nonet(so);
>  
>  	if (soqremque(so, 1) == 0)
>  		panic("accept");
> @@ -353,7 +325,7 @@ doaccept(struct proc *p, int sock, struc
>  	/* connection has been removed from the listen queue */
>  	knote(&head->so_rcv.sb_klist, 0);
>  
> -	doaccept_sounlock(head, 0);
> +	sounlock_nonet(head);
>  
>  	fp->f_type = DTYPE_SOCKET;
>  	fp->f_flag = FREAD | FWRITE | nflag;
> @@ -362,7 +334,7 @@ doaccept(struct proc *p, int sock, struc
>  
>  	error = soaccept(so, nam);
>  
> -	doaccept_sounlock(so, 1);
> +	sounlock_shared(so);
>  
>  	if (error)
>  		goto out;
> @@ -385,7 +357,7 @@ doaccept(struct proc *p, int sock, struc
>  	return 0;
>  
>  out_unlock:
> -	doaccept_sounlock(head, 1);
> +	sounlock_shared(head);
>  out:
>  	fdplock(fdp);
>  	fdremove(fdp, tmpfd);
> Index: sys/socketvar.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/socketvar.h,v
> diff -u -p -r1.138 socketvar.h
> --- sys/socketvar.h	3 Jan 2025 12:56:15 -0000	1.138
> +++ sys/socketvar.h	16 Jan 2025 13:01:34 -0000
> @@ -451,10 +451,12 @@ int	sockargs(struct mbuf **, const void 
>  int	sosleep_nsec(struct socket *, void *, int, const char *, uint64_t);
>  void	solock(struct socket *);
>  void	solock_shared(struct socket *);
> +void	solock_nonet(struct socket *);
>  int	solock_persocket(struct socket *);
>  void	solock_pair(struct socket *, struct socket *);
>  void	sounlock(struct socket *);
>  void	sounlock_shared(struct socket *);
> +void	sounlock_nonet(struct socket *);
>  
>  int	sendit(struct proc *, int, struct msghdr *, int, register_t *);
>  int	recvit(struct proc *, int, struct msghdr *, caddr_t, register_t *);
>