svn commit: r263755 - head/sys/kern

[Don Lewis]

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:
 	/*