On Tue, Jul 18, 2017 at 09:03:00AM +0200, Martin Pieuchot wrote:
> Diff below extends the scope of the socket lock. It has been previously
> introduced in sys_connect(), first as NET_LOCK() then renamed, where old
> splsofnet() were used. But we now need to "move the line up" in order to
> protect fields currently relying on the KERNEL_LOCK().
We were also discussing to move the lock down to the network stack.
Eventually we need locks for the sockets and a lock for the stack.
The first must move up and the latter down. As we have only one
lock at the moment, I am fine with the upward direction for now.
> Moving the line up means that soconnect() and soconnect2() will now
> assert for the socket lock.
The soconnect2() is only used for local sockets that run with the
kernel lock. So we could not lock this function. I guess your
goal is per socket locks, so your change is fine.
> @@ -373,9 +373,10 @@ sys_connect(struct proc *p, void *v, reg
> if ((error = getsock(p, SCARG(uap, s), &fp)) != 0)
> return (error);
> so = fp->f_data;
> + s = solock(so);
> if (so->so_state & SS_ISCONNECTING) {
> - FRELE(fp, p);
> - return (EALREADY);
> + error = EALREADY;
> + goto out;
> }
> error = sockargs(&nam, SCARG(uap, name), SCARG(uap, namelen),
> MT_SONAME);
> @@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, reg
> if (isdnssocket(so)) {
> u_int namelen = nam->m_len;
> error = dns_portcheck(p, so, mtod(nam, void *), &namelen);
> - if (error) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (error);
> - }
> + if (error)
> + goto out;
> nam->m_len = namelen;
> }
>
Now we have a mixture fo goto bad and goto out. I think everything
before soconnect() should goto out. Please change the error handling
of sockargs() and pledge_socket() to goto out.
> @@ -135,6 +135,11 @@ fifo_open(void *v)
> return (error);
> }
> fip->fi_readsock = rso;
> + /*
> + * XXXSMP
> + * We only lock `wso' because AF_LOCAL sockets are
> + * still relying on the KERNEL_LOCK().
> + */
> if ((error = socreate(AF_LOCAL, &wso, SOCK_STREAM, 0)) != 0) {
> (void)soclose(rso);
> free(fip, M_VNODE, sizeof *fip);
Could you move this comment before the solock(wso) line?
> @@ -168,11 +179,12 @@ fifo_open(void *v)
> goto bad;
> }
> if (fip->fi_writers == 1) {
> - fip->fi_readsock->so_state &=
> ~(SS_CANTRCVMORE|SS_ISDISCONNECTED);
> + rso->so_state &= ~(SS_CANTRCVMORE|SS_ISDISCONNECTED);
> if (fip->fi_readers > 0)
> wakeup(&fip->fi_readers);
> }
> }
> + sounlock(s);
> if ((ap->a_mode & O_NONBLOCK) == 0) {
> if ((ap->a_mode & FREAD) && fip->fi_writers == 0) {
> VOP_UNLOCK(vp, p);
The goto bad after error = ENXIO has no sounlock().
bluhm
