Date: Thu, 27 Mar 2014 15:31:30 -0700 (PDT) From: Don Lewis <truckman@FreeBSD.org> To: kostikbel@gmail.com Cc: mjguzik@gmail.com, mjg@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, davidxu@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <201403272231.s2RMVUu0008735@gw.catspoiler.org> In-Reply-To: <20140327162346.GR21331@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201403272231.s2RMVUu0008735>