Date: Fri, 28 Mar 2014 14:26:47 +0800 From: David Xu <davidxu@freebsd.org> To: Don Lewis <truckman@FreeBSD.org>, kostikbel@gmail.com Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, mjguzik@gmail.com, src-committers@FreeBSD.org, mjg@FreeBSD.org Subject: Re: svn commit: r263755 - head/sys/kern Message-ID: <53351627.9000703@freebsd.org> In-Reply-To: <201403272231.s2RMVUu0008735@gw.catspoiler.org> References: <201403272231.s2RMVUu0008735@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
> /*
>
>
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:
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53351627.9000703>