From owner-svn-src-head@FreeBSD.ORG Sat Mar 29 02:56:11 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 2DEAF3EF; Sat, 29 Mar 2014 02:56:11 +0000 (UTC) Received: from mail-wi0-x22d.google.com (mail-wi0-x22d.google.com [IPv6:2a00:1450:400c:c05::22d]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 012138E2; Sat, 29 Mar 2014 02:56:09 +0000 (UTC) Received: by mail-wi0-f173.google.com with SMTP id f8so1410327wiw.6 for ; Fri, 28 Mar 2014 19:56:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=Xwko+P9hVoBZo8IrtmsiexAh/aAt2k73WiRPcMwpJPg=; b=GgcYiy28vajYYNQB6k7kRKpfioHwic7URWTQyUQhsah74k4T6fxEgwf2UzLGHocwGK T0ZyFpwO0ZC6nZeltPOHz+9qnU3AFrke6gQXBOh7cwvnLUC3jCp2VFPMvj/Ckx1pBGdD sCdmDrwHA34acwCLtsfIZPR2r7Tb0UhZpxbSpmwo8ZbtXViEoVlkUUdAbu0f52PPJf/g CCEjAoEelT0obWyLO9ePKnKyQLbwhgkX00CiJBX0L7adaoPZDykOT6k9kwdgKK4WmaND jEviCaWDV0Bi7vKB1pwPbt2W8kOKehXocOHfrpmau7TO2Ypp5MBQOukIn5nDB0zZO0i0 0lJw== X-Received: by 10.180.77.74 with SMTP id q10mr137904wiw.39.1396061768170; Fri, 28 Mar 2014 19:56:08 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id er4sm1415669wjd.38.2014.03.28.19.56.05 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 28 Mar 2014 19:56:05 -0700 (PDT) Date: Sat, 29 Mar 2014 03:56:02 +0100 From: Mateusz Guzik To: Don Lewis Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <20140329025602.GB29296@dft-labs.eu> References: <53351627.9000703@freebsd.org> <201403281613.s2SGDKpk010871@gw.catspoiler.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201403281613.s2SGDKpk010871@gw.catspoiler.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: src-committers@FreeBSD.org, mjg@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org, davidxu@FreeBSD.org, kostikbel@gmail.com 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: Sat, 29 Mar 2014 02:56:11 -0000 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