From owner-svn-src-all@FreeBSD.ORG Thu Mar 27 22:31:49 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 1EFDA3DA; Thu, 27 Mar 2014 22:31:49 +0000 (UTC) Received: from gw.catspoiler.org (gw.catspoiler.org [75.1.14.242]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id EAC43F4C; Thu, 27 Mar 2014 22:31:48 +0000 (UTC) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id s2RMVUu0008735; Thu, 27 Mar 2014 14:31:34 -0800 (PST) (envelope-from truckman@FreeBSD.org) Message-Id: <201403272231.s2RMVUu0008735@gw.catspoiler.org> Date: Thu, 27 Mar 2014 15:31:30 -0700 (PDT) From: Don Lewis Subject: Re: svn commit: r263755 - head/sys/kern To: kostikbel@gmail.com In-Reply-To: <20140327162346.GR21331@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii Cc: mjguzik@gmail.com, mjg@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, davidxu@FreeBSD.org, svn-src-head@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: Thu, 27 Mar 2014 22:31:49 -0000 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: /*