From owner-svn-src-all@FreeBSD.ORG Fri Mar 28 01:47:32 2014 Return-Path: Delivered-To: svn-src-all@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 C16D4604; Fri, 28 Mar 2014 01:47:32 +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 AB725684; Fri, 28 Mar 2014 01:47:32 +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 s2S1lUQr025173; Fri, 28 Mar 2014 01:47:30 GMT (envelope-from davidxu@freebsd.org) Message-ID: <5334D4DF.4050901@freebsd.org> Date: Fri, 28 Mar 2014 09:48:15 +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: Don Lewis , kostikbel@gmail.com Subject: Re: svn commit: r263755 - head/sys/kern References: <201403272231.s2RMVUu0008735@gw.catspoiler.org> In-Reply-To: <201403272231.s2RMVUu0008735@gw.catspoiler.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, mjguzik@gmail.com, 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: Fri, 28 Mar 2014 01:47:32 -0000 On 2014/03/28 06:31, Don Lewis wrote: > On 27 Mar, Konstantin Belousov wrote: >> On Thu, Mar 27, 2014 at 04:05:12PM +0100, Mateusz Guzik wrote: >>> On Thu, Mar 27, 2014 at 03:58:19PM +0100, Mateusz Guzik wrote: >>>> On Thu, Mar 27, 2014 at 04:46:57PM +0800, David Xu wrote: >>>>> On 2014/03/27 16:37, Mateusz Guzik wrote: >>>>>> On Thu, Mar 27, 2014 at 03:45:17PM +0800, David Xu wrote: >>>>>>> I think the async process pointer can be cleared when a process exits >>>>>>> by registering an event handler. please see attached patch. >>>>>>> >>>>>> >>>>>> Sure, but I'm not very fond of this solution. >>>>>> >>>>>> This is a rather obscure bug you wont hit unless you explicitly try, >>>>>> and even then you need root privs by default. >>>>>> >>>>> OK, but I don't like the bug exists in kernel. It is not obscure for me, >>>>> I can run "shutdown now" command, and insert a device, and then the >>>>> kernel will write garbage data into freed memory space. >>>>> >>>> >>>> Not sure what you mean. devd does not use this feature, and even if it >>>> did async_proc is cleared on close, which happens while signal delivery >>>> is still legal. >>>> >>>> That said, you are not going to encounter this bug unless you code >>>> something up to specifically trigger it. >>>> >>>> fwiw, I think we could axe this feature if there was no way to fix it >>>> without introducing a check for every process. >>>> >>>>>> As such writing a callback function which will be executed for all exiting >>>>>> processes seems unjustified for me. >>>>>> >>>>>> Ideally we would get some mechanism which would allow to register >>>>>> callbacks for events related to given entity. Then it could be used to >>>>>> provide a "call this function when process p exits", amongst other things. >>>>>> >>>>> >>>>> Yes, but the callback itself is cheap enough and is not worth to be >>>>> per-entity entry. >>>>> >>>> >>>> There is other code in the kernel which would benefit from such >>>> functionality - dev/syscons/scmouse, dev/vt/vt_core.c, aio and possibly >>>> more. >>>> >>>> As such I think this is worth pursuing. >>>> >>> >>> We can hack around this one the way the other code is doing - apart from >>> from proc pointer you store pid and then compare result of pfind(pid). >>> >>> This is still buggy as both proc and pid pointer can be recycled and end >>> up being the same (but you have an entrirely new process). >>> >>> However, then in absolutely worst cae you send SIGIO to incorrect >>> process, always an existing process so no more corruption. >>> >>> Would you be ok with such hack for the time being? >> >> Isn't p_sigiolist and fsetown(9) already provide the neccessary registration >> and cleanup on the process exit ? The KPI might require some generalization, >> but I think that the mechanism itself is enough. > > That's the correct mechanism, but it's not being used here. > > Something like the following untested patch should do the trick: > > Index: sys/kern/subr_bus.c > =================================================================== > --- sys/kern/subr_bus.c (revision 263289) > +++ sys/kern/subr_bus.c (working copy) > @@ -402,7 +402,7 @@ > struct cv cv; > struct selinfo sel; > struct devq devq; > - struct proc *async_proc; > + struct sigio *sigio; > } devsoftc; > > static struct cdev *devctl_dev; > @@ -425,7 +425,7 @@ > /* move to init */ > devsoftc.inuse = 1; > devsoftc.nonblock = 0; > - devsoftc.async_proc = NULL; > + funsetown(&devsoftc.sigio); > return (0); > } > > @@ -436,7 +436,7 @@ > mtx_lock(&devsoftc.mtx); > cv_broadcast(&devsoftc.cv); > mtx_unlock(&devsoftc.mtx); > - devsoftc.async_proc = NULL; > + funsetown(&devsoftc.sigio); > return (0); > } > > @@ -492,9 +492,8 @@ > return (0); > case FIOASYNC: > if (*(int*)data) > - devsoftc.async_proc = td->td_proc; > - else > - devsoftc.async_proc = NULL; > + return (fsetown(td->td_proc->p_pid, &devsoftc.sigio)); > + funsetown(&devsoftc.sigio); > return (0); > > /* (un)Support for other fcntl() calls. */ > @@ -546,7 +545,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; > @@ -576,12 +574,8 @@ > cv_broadcast(&devsoftc.cv); > mtx_unlock(&devsoftc.mtx); > selwakeup(&devsoftc.sel); > - p = devsoftc.async_proc; > - if (p != NULL) { > - PROC_LOCK(p); > - kern_psignal(p, SIGIO); > - PROC_UNLOCK(p); > - } > + if (devsoftc.sigio != NULL) > + pgsigio(&devsoftc.sigio, SIGIO, 0); > return; > out: > /* > > Hope this works.