From: Vitaliy Makkoveev Subject: Re: lock sonewconn() for internet sockets To: Alexander Bluhm Cc: tech@openbsd.org Date: Thu, 16 Jan 2025 16:33:41 +0300 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 *); >