Date: Sat, 29 Mar 2014 03:56:02 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Don Lewis <truckman@FreeBSD.org> Cc: src-committers@FreeBSD.org, mjg@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org, davidxu@FreeBSD.org, kostikbel@gmail.com Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <20140329025602.GB29296@dft-labs.eu> In-Reply-To: <201403281613.s2SGDKpk010871@gw.catspoiler.org> References: <53351627.9000703@freebsd.org> <201403281613.s2SGDKpk010871@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140329025602.GB29296>