svn commit: r263755 - head/sys/kern

[Mateusz Guzik]

On Fri, Mar 28, 2014 at 09:13:20AM -0700, Don Lewis wrote:
> On 28 Mar, David Xu wrote:
> > I have tweaked it a bit, is this okay ?
> > 
> > # HG changeset patch
> > # Parent 53b614ff2cae108f27e4475989d3a86997017268
> > 
> > diff -r 53b614ff2cae sys/kern/subr_bus.c
> > --- a/sys/kern/subr_bus.c	Thu Mar 27 10:03:50 2014 +0800
> > +++ b/sys/kern/subr_bus.c	Fri Mar 28 14:22:29 2014 +0800
> > @@ -391,11 +391,12 @@
> >   	int	inuse;
> >   	int	nonblock;
> >   	int	queued;
> > +	int	async;
> >   	struct mtx mtx;
> >   	struct cv cv;
> >   	struct selinfo sel;
> >   	struct devq devq;
> > -	struct proc *async_proc;
> > +	struct sigio *sigio;
> >   } devsoftc;
> > 
> >   static struct cdev *devctl_dev;
> > @@ -422,7 +423,8 @@
> >   	/* move to init */
> >   	devsoftc.inuse = 1;
> >   	devsoftc.nonblock = 0;
> > -	devsoftc.async_proc = NULL;
> > +	devsoftc.async = 0;
> > +	devsoftc.sigio = NULL;
> >   	mtx_unlock(&devsoftc.mtx);
> >   	return (0);
> >   }
> > @@ -433,8 +435,9 @@
> > 
> >   	mtx_lock(&devsoftc.mtx);
> >   	devsoftc.inuse = 0;
> > -	devsoftc.async_proc = NULL;
> > +	devsoftc.async = 0;
> >   	cv_broadcast(&devsoftc.cv);
> > +	funsetown(&devsoftc.sigio);
> >   	mtx_unlock(&devsoftc.mtx);
> >   	return (0);
> >   }
> > @@ -490,33 +493,21 @@
> >   			devsoftc.nonblock = 0;
> >   		return (0);
> >   	case FIOASYNC:
> > -		/*
> > -		 * FIXME:
> > -		 * Since this is a simple assignment there is no guarantee that
> > -		 * devsoftc.async_proc consumers will get a valid pointer.
> > -		 *
> > -		 * Example scenario where things break (processes A and B):
> > -		 * 1. A opens devctl
> > -		 * 2. A sends fd to B
> > -		 * 3. B sets itself as async_proc
> > -		 * 4. B exits
> > -		 *
> > -		 * However, normally this requires root privileges and the only
> > -		 * in-tree consumer does not behave in a dangerous way so the
> > -		 * issue is not critical.
> > -		 */
> >   		if (*(int*)data)
> > -			devsoftc.async_proc = td->td_proc;
> > +			devsoftc.async = 1;
> >   		else
> > -			devsoftc.async_proc = NULL;
> > +			devsoftc.async = 0;
> > +		return (0);
> > +	case FIOSETOWN:
> > +		return fsetown(*(int *)data, &devsoftc.sigio);
> > +	case FIOGETOWN:
> > +		*(int *)data = fgetown(&devsoftc.sigio);
> >   		return (0);
> > 
> >   		/* (un)Support for other fcntl() calls. */
> >   	case FIOCLEX:
> >   	case FIONCLEX:
> >   	case FIONREAD:
> > -	case FIOSETOWN:
> > -	case FIOGETOWN:
> >   	default:
> >   		break;
> >   	}
> > @@ -560,7 +551,6 @@
> >   devctl_queue_data_f(char *data, int flags)
> >   {
> >   	struct dev_event_info *n1 = NULL, *n2 = NULL;
> > -	struct proc *p;
> > 
> >   	if (strlen(data) == 0)
> >   		goto out;
> > @@ -590,13 +580,8 @@
> >   	cv_broadcast(&devsoftc.cv);
> >   	mtx_unlock(&devsoftc.mtx);
> >   	selwakeup(&devsoftc.sel);
> > -	/* XXX see a comment in devioctl */
> > -	p = devsoftc.async_proc;
> > -	if (p != NULL) {
> > -		PROC_LOCK(p);
> > -		kern_psignal(p, SIGIO);
> > -		PROC_UNLOCK(p);
> > -	}
> > +	if (devsoftc.async && devsoftc.sigio != NULL)
> > +		pgsigio(&devsoftc.sigio, SIGIO, 0);
> >   	return;
> >   out:
> >   	/*
> > 
> > 
> 
> That makes it work more like the other users of fsetown(), which is
> probably a good thing.  The downside is that two syscalls are needed to
> activate it, which I was trying to avoid that with my patch.  I noticed
> that logopen() in subr_log.c unconditionally calls fsetown(), which
> would avoid the need for an extra syscall.  That also avoids the direct
> manipulation of the pointer in your patch, which makes me nervous about
> the possibility of a leak.
> 
> I wonder if FIOASYNC should fail if
> 	td->td_proc != devsoftc.sigio.sio_proc
> (or the equivalent for other instances) to prevent a process from
> maniuplating the async flag for a device "owned" by another process.  I
> think this check would need to be wrapped in SIGIO_LOCK()/SIGIO_UNLOCK()
> to be safe.
> 

But this patch would mean that current consumers (if any) would break -
just calling FIOASYNC would not result in receiving SIGIO.

Original patch by Don seems to work fine though, but I'm unsure about
one thing (present in this patch as well):

There is one devsoftc.sigio instance and one can get multiple processes
with devctl fd. Is it safe from kernel perspective to have multiple
processes call fsetown(*(int *)data, &devsoftc.sigio)?

-- 
Mateusz Guzik <mjguzik gmail.com>