From owner-svn-src-all@FreeBSD.ORG Sat Mar 29 03:09:40 2014 Return-Path: Delivered-To: svn-src-all@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 22A68B1E; Sat, 29 Mar 2014 03:09:40 +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 02635A65; Sat, 29 Mar 2014 03:09:40 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.8/8.14.8) with ESMTP id s2T39afq028048; Sat, 29 Mar 2014 03:09:36 GMT (envelope-from davidxu@freebsd.org) Message-ID: <5336396E.7000801@freebsd.org> Date: Sat, 29 Mar 2014 11:09:34 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Mateusz Guzik , Don Lewis Subject: Re: svn commit: r263755 - head/sys/kern References: <53351627.9000703@freebsd.org> <201403281613.s2SGDKpk010871@gw.catspoiler.org> <20140329025602.GB29296@dft-labs.eu> In-Reply-To: <20140329025602.GB29296@dft-labs.eu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kostikbel@gmail.com, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, mjg@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 29 Mar 2014 03:09:40 -0000 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.