Fix a pair of bugs in fsetown()

[markj]

Summary

- pget()/pfind() will acquire the PID hash bucket locks, which are
  sleepable locks, but this means that the sigio mutex cannot be held
  while calling these functions.  Instead, use pget() to hold the
  process, after which we lock the sigio and proc locks, respectively.
- funsetownlst() assumes that processes cannot be registered for SIGIO
  once they have P_WEXIT set.  However, pfind() will happily return
  exiting processes.  Add an explicit check for P_WEXIT in fsetown() to
  fix this.

Fixes: f52979098d3c ("Fix a pair of races in SIGIO registration")
Reported by: syzkaller

[kib]

As a next step, I would restructure fsetown() even more. I suggest to not unlock proc/proctree and not to goto to fail. Instead do something along these lines:

      if (pgid > 0) {
                ret = pget(pgid, PGET_NOTWEXIT | PGET_NOTID | PGET_HOLD, &proc);
                SIGIO_LOCK();
                if (ret == 0) {
			osigio = funsetown_locked(*sigiop);
	                PROC_LOCK(proc);
	                if ((proc->p_flag & P_WEXIT) != 0) {
				ret = ESRCH;
			} else {
				/*
				 * Policy - Don't allow a process to FSETOWN a process
				 * in another session.
				 *
				 * Remove this test to allow maximum flexibility or
				 * restrict FSETOWN to the current process or process
				 * group for maximum safety.
				 */
				if (proc->p_session != curthread->td_proc->p_session) {
					ret = EPERM;
				} else {
					sigio->sio_proc = proc;
					SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio);
				}
			}
			_PRELE(proc);
			PROC_UNLOCK(proc);
		}
        } else /* if (pgid < 0) */ {
                sx_slock(&proctree_lock);
                SIGIO_LOCK();
                pgrp = pgfind(-pgid);
                if (pgrp != NULL) {
                        ret = ESRCH;
                } else {
			osigio = funsetown_locked(*sigiop);

			/*
			 * Policy - Don't allow a process to FSETOWN a process
			 * in another session.
			 *
			 * Remove this test to allow maximum flexibility or
			 * restrict FSETOWN to the current process or process
			 * group for maximum safety.
			 */
			if (pgrp->pg_session != curthread->td_proc->p_session) {
				ret = EPERM;
			} else {
				sigio->sio_pgrp = pgrp;
				SLIST_INSERT_HEAD(&pgrp->pg_sigiolst, sigio, sio_pgsigio);
			}
			PGRP_UNLOCK(pgrp);
		}
		sx_sunlock(&proctree_lock);
        }
	if (ret == 0)
		*sigiop = sigio;
        SIGIO_UNLOCK();
        if (osigio != NULL)
                sigiofree(osigio);
        return (ret);

I myself prefer lesser indent level, but IMO gathering all unlocks in single place worth it.

[markj]

In D31661#714118, @kib wrote:

As a next step, I would restructure fsetown() even more. I suggest to not unlock proc/proctree and not to goto to fail. Instead do something along these lines:

Thanks, I think your suggestion is better. I mostly followed the suggestion but tried to avoid quadruple-nesting: D31671.