From owner-svn-src-head@FreeBSD.ORG Thu Mar 27 07:44:35 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1A523AF5; Thu, 27 Mar 2014 07:44:35 +0000 (UTC) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 03268B78; Thu, 27 Mar 2014 07:44:35 +0000 (UTC) Received: from xyf.my.dom (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.8/8.14.8) with ESMTP id s2R7iWBv077098; Thu, 27 Mar 2014 07:44:33 GMT (envelope-from davidxu@freebsd.org) Message-ID: <5333D70D.7050306@freebsd.org> Date: Thu, 27 Mar 2014 15:45:17 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; FreeBSD i386; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: Mateusz Guzik , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r263755 - head/sys/kern References: <201403252330.s2PNUaei052956@svn.freebsd.org> In-Reply-To: <201403252330.s2PNUaei052956@svn.freebsd.org> Content-Type: multipart/mixed; boundary="------------000008090800090909020404" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Mar 2014 07:44:35 -0000 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 #include #include +#include #include @@ -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--