Date: Thu, 27 Mar 2014 15:45:17 +0800 From: David Xu <davidxu@freebsd.org> To: Mateusz Guzik <mjg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <5333D70D.7050306@freebsd.org> In-Reply-To: <201403252330.s2PNUaei052956@svn.freebsd.org> References: <201403252330.s2PNUaei052956@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------000008090800090909020404 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2014/03/26 07:30, Mateusz Guzik wrote: > Author: mjg > Date: Tue Mar 25 23:30:35 2014 > New Revision: 263755 > URL: http://svnweb.freebsd.org/changeset/base/263755 > > Log: > Document a known problem with handling the process intended to receive > SIGIO in /dev/devctl. > > Suggested by: adrian > MFC after: 6 days > > Modified: > head/sys/kern/subr_bus.c > > Modified: head/sys/kern/subr_bus.c > ============================================================================== > --- head/sys/kern/subr_bus.c Tue Mar 25 23:19:45 2014 (r263754) > +++ head/sys/kern/subr_bus.c Tue Mar 25 23:30:35 2014 (r263755) > @@ -490,6 +490,21 @@ devioctl(struct cdev *dev, u_long cmd, c > 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; > else > @@ -575,6 +590,7 @@ devctl_queue_data_f(char *data, int flag > 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); > > I think the async process pointer can be cleared when a process exits by registering an event handler. please see attached patch. --------------000008090800090909020404 Content-Type: text/x-patch; name="dev_sigio.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="dev_sigio.patch" # 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 Thu Mar 27 15:38:40 2014 +0800 @@ -54,6 +54,7 @@ #include <sys/uio.h> #include <sys/bus.h> #include <sys/interrupt.h> +#include <sys/eventhandler.h> #include <net/vnet.h> @@ -399,6 +400,18 @@ } devsoftc; static struct cdev *devctl_dev; +static eventhandler_tag devctl_eh; + +static void +devctl_proc_clear(void *arg, struct proc *p, struct image_params *imgp __unused) +{ + if (devsoftc.async_proc == p) { + mtx_lock(&devsoftc.mtx); + if (devsoftc.async_proc == p) + devsoftc.async_proc = NULL; + mtx_unlock(&devsoftc.mtx); + } +} static void devinit(void) @@ -408,6 +421,8 @@ mtx_init(&devsoftc.mtx, "dev mtx", "devd", MTX_DEF); cv_init(&devsoftc.cv, "dev cv"); TAILQ_INIT(&devsoftc.devq); + devctl_eh = EVENTHANDLER_REGISTER(process_exit, devctl_proc_clear, NULL, + EVENTHANDLER_PRI_ANY); } static int @@ -490,25 +505,12 @@ 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. - */ + mtx_lock(&devsoftc.mtx); if (*(int*)data) devsoftc.async_proc = td->td_proc; else devsoftc.async_proc = NULL; + mtx_unlock(&devsoftc.mtx); return (0); /* (un)Support for other fcntl() calls. */ @@ -588,15 +590,14 @@ TAILQ_INSERT_TAIL(&devsoftc.devq, n1, dei_link); devsoftc.queued++; 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); } + mtx_unlock(&devsoftc.mtx); return; out: /* @@ -4503,7 +4504,7 @@ return (0); case MOD_SHUTDOWN: - device_shutdown(root_bus); + EVENTHANDLER_DEREGISTER(process_exit, devctl_eh); return (0); default: return (EOPNOTSUPP); --------------000008090800090909020404--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5333D70D.7050306>