Date: Sat, 29 Mar 2014 11:09:34 +0800 From: David Xu <davidxu@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com>, Don Lewis <truckman@FreeBSD.org> Cc: kostikbel@gmail.com, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, mjg@FreeBSD.org Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <5336396E.7000801@freebsd.org> In-Reply-To: <20140329025602.GB29296@dft-labs.eu> References: <53351627.9000703@freebsd.org> <201403281613.s2SGDKpk010871@gw.catspoiler.org> <20140329025602.GB29296@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2014/03/29 10:56, Mateusz Guzik wrote:
> 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.
The old behavior is inconsistent with other piece of code in the kernel and
may be incompatible with POSIX.
> 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)?
>
There is an inuse variable guarding this problem, you can not open it
multiple times, you can only do it in the forked process which inherited
the fd. if you don't trust the child process, you can close it before
executing real code in the child process.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5336396E.7000801>