On 01/07/21(Thu) 21:27, Alexander Bluhm wrote:
> Hi,
>
> Writing ktrace files to NFS must no be done while holding the net
> lock. accept(2) panics, connect(2) dead locks. Additionally copy
> in or out must not hold the net lock as it may be a mmapped file
> on NFS.
>
> - Simplify dns_portcheck(), it does not modify namelen anymore.
> - In doaccept() release the socket lock before calling copyaddrout().
> - Rearrange the checks in sys_connect() like they are in sys_bind().
Looks good to me. Grabbing solock() after calling pledge_socket() in
sys_connect(), like it is already done in sys_bind(), means it is ok
to read this field w/o lock. Is it true?
ok mpi@ for this diff because it makes things coherent and fix a bug,
then we can figure out and document the truth about `so_state'.
> Index: kern/uipc_syscalls.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.192
> diff -u -p -r1.192 uipc_syscalls.c
> --- kern/uipc_syscalls.c 2 Jun 2021 11:30:23 -0000 1.192
> +++ kern/uipc_syscalls.c 1 Jul 2021 18:34:21 -0000
> @@ -124,20 +124,20 @@ isdnssocket(struct socket *so)
>
> /* For SS_DNS sockets, only allow port DNS (port 53) */
> static int
> -dns_portcheck(struct proc *p, struct socket *so, void *nam, u_int *namelen)
> +dns_portcheck(struct proc *p, struct socket *so, void *nam, size_t namelen)
> {
> int error = EINVAL;
>
> switch (so->so_proto->pr_domain->dom_family) {
> case AF_INET:
> - if (*namelen < sizeof(struct sockaddr_in))
> + if (namelen < sizeof(struct sockaddr_in))
> break;
> if (((struct sockaddr_in *)nam)->sin_port == htons(53))
> error = 0;
> break;
> #ifdef INET6
> case AF_INET6:
> - if (*namelen < sizeof(struct sockaddr_in6))
> + if (namelen < sizeof(struct sockaddr_in6))
> break;
> if (((struct sockaddr_in6 *)nam)->sin6_port == htons(53))
> error = 0;
> @@ -315,18 +315,17 @@ doaccept(struct proc *p, int sock, struc
> fp->f_ops = &socketops;
> fp->f_data = so;
> error = soaccept(so, nam);
> +out:
> + sounlock(head, s);
> if (!error && name != NULL)
> error = copyaddrout(p, nam, name, namelen, anamelen);
> -out:
> if (!error) {
> - sounlock(head, s);
> fdplock(fdp);
> fdinsert(fdp, tmpfd, cloexec, fp);
> fdpunlock(fdp);
> FRELE(fp, p);
> *retval = tmpfd;
> } else {
> - sounlock(head, s);
> fdplock(fdp);
> fdremove(fdp, tmpfd);
> fdpunlock(fdp);
> @@ -348,44 +347,40 @@ sys_connect(struct proc *p, void *v, reg
> } */ *uap = v;
> struct file *fp;
> struct socket *so;
> - struct mbuf *nam = NULL;
> + struct mbuf *nam;
> int error, s, interrupted = 0;
>
> if ((error = getsock(p, SCARG(uap, s), &fp)) != 0)
> return (error);
> so = fp->f_data;
> - s = solock(so);
> - if (so->so_state & SS_ISCONNECTING) {
> - error = EALREADY;
> + error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
> + so->so_state);
> + if (error)
> goto out;
> - }
> error = sockargs(&nam, SCARG(uap, name), SCARG(uap, namelen),
> MT_SONAME);
> if (error)
> goto out;
> - error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
> - so->so_state);
> - if (error)
> - goto out;
> #ifdef KTRACE
> if (KTRPOINT(p, KTR_STRUCT))
> ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
> #endif
> -
> + s = solock(so);
> if (isdnssocket(so)) {
> - u_int namelen = nam->m_len;
> - error = dns_portcheck(p, so, mtod(nam, void *), &namelen);
> + error = dns_portcheck(p, so, mtod(nam, void *), nam->m_len);
> if (error)
> - goto out;
> - nam->m_len = namelen;
> + goto unlock;
> + }
> + if (so->so_state & SS_ISCONNECTING) {
> + error = EALREADY;
> + goto unlock;
> }
> -
> error = soconnect(so, nam);
> if (error)
> goto bad;
> if ((fp->f_flag & FNONBLOCK) && (so->so_state & SS_ISCONNECTING)) {
> error = EINPROGRESS;
> - goto out;
> + goto unlock;
> }
> while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
> error = sosleep_nsec(so, &so->so_timeo, PSOCK | PCATCH,
> @@ -403,10 +398,11 @@ sys_connect(struct proc *p, void *v, reg
> bad:
> if (!interrupted)
> so->so_state &= ~SS_ISCONNECTING;
> -out:
> +unlock:
> sounlock(so, s);
> - FRELE(fp, p);
> m_freem(nam);
> +out:
> + FRELE(fp, p);
> if (error == ERESTART)
> error = EINTR;
> return (error);
> @@ -618,12 +614,10 @@ sendit(struct proc *p, int s, struct msg
> if (error)
> goto bad;
> if (isdnssocket(so)) {
> - u_int namelen = mp->msg_namelen;
> error = dns_portcheck(p, so, mtod(to, caddr_t),
> - &namelen);
> + mp->msg_namelen);
> if (error)
> goto bad;
> - mp->msg_namelen = namelen;
> }
> #ifdef KTRACE
> if (KTRPOINT(p, KTR_STRUCT))
>
