freebsd stable - Oct 2004 - panic caused by EVFILT_SIGNAL detaching in rfork()ed thread

Here is more correct patch to fix the panic in 4.x reported in
http://freebsd.rambler.ru/bsdmail/freebsd-hackers_2004/msg02732.html

-------------------------
--- src/sys/kern/kern_event.c   Sun Oct 10 12:17:55 2004
+++ src/sys/kern/kern_event.c   Sun Oct 10 12:19:29 2004
@@ -794,7 +794,8 @@
            while (kn != NULL) {
                kn0 = SLIST_NEXT(kn, kn_link);
                if (kq == kn->kn_kq) {
-                   kn->kn_fop->f_detach(kn);
+                   if (!(kn->kn_status & KN_DETACHED))
+                       kn->kn_fop->f_detach(kn);
        /* XXX non-fd release of kn->kn_ptr */
                    knote_free(kn);
                    *knp = kn0;
-------------------------

The patch based on the fix for FreeBSD 5.x:
http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/kern/kern_event.c.diff?r1=1.79&r2=1.80

For more information see the thread started in
http://freebsd.rambler.ru/bsdmail/freebsd-current_2004/msg18389.html

To reproduce the panic in 4.x you could download
http://sysoev.ru/nginx/nginx-0.1.2.tar.gz
and build it without the installation:

tar zxf nginx-0.1.2.tar.gz
cd nginx-0.1.2
./configure --with-threads=rfork \
            --without-http_rewrite_module \
            --prefix=$PWD \
            --pid-path=nginx.pid \
            --http-log-path=access.log \
            --error-log-path=stderr
make

Now you have to edit ./conf/nginx.conf.

Add the line "daemon  off;" in the start of the file.
Change the lines "worker_processes  3;" to "worker_threads 
3;"
and "listen 80;" to "listen  8000;".

Now you could run ./nginx as non-privileged user.

If you run in another console

ps ax -o pid,ppid,%cpu,vsz,wchan,command|egrep '(nginx:|PID)'

you will see something like this:

  PID  PPID %CPU   VSZ WCHAN  COMMAND
 3737   238  0.0  1340 pause  nginx: master process ./nginx
 3738  3737  0.0  1844 kqread nginx: worker process (nginx)
 3739  3738  0.0  1844 kqread nginx: worker thread (nginx)
 3740  3738  0.0  1844 kqread nginx: worker thread (nginx)
 3741  3738  0.0  1844 kqread nginx: worker thread (nginx)

Now stop nginx by pressing ^C. Your system may panic.


Igor Sysoev
http://sysoev.ru/en/

Igor Sysoev wrote:
> Here is more correct patch to fix the panic in 4.x reported in
> http://freebsd.rambler.ru/bsdmail/freebsd-hackers_2004/msg02732.html
> 
> -------------------------
> --- src/sys/kern/kern_event.c   Sun Oct 10 12:17:55 2004
> +++ src/sys/kern/kern_event.c   Sun Oct 10 12:19:29 2004
> @@ -794,7 +794,8 @@
>             while (kn != NULL) {
>                 kn0 = SLIST_NEXT(kn, kn_link);
>                 if (kq == kn->kn_kq) {
> -                   kn->kn_fop->f_detach(kn);
> +                   if (!(kn->kn_status & KN_DETACHED))
> +                       kn->kn_fop->f_detach(kn);
>         /* XXX non-fd release of kn->kn_ptr */
>                     knote_free(kn);
>                     *knp = kn0;
> -------------------------
Your patch appears to be an excerpt from the fix to RELENG_5.  May I 
suggest a different approach for RELENG_4?  My reasoning is that the 
implementation of kevents differs between RELENG_4 and RELENG_5.

In RELENG_5 the flag KN_DETACHED is used in a more general way.  It gets 
set by knlist_add() and cleared by knlist_remove(), in sync with list 
insertion and removal.  As far as I can tell these routines have 
originally been introduced in order to centralize the locking for kevent 
list manipulations, and they don't exist in RELENG_4.

Now, the proper way to MFC the RELENG_5 fix to RELENG_4 more or less 
unchanged would be to MFC the whole knlist_add()/knlist_remove() 
business as well (w/o the locking stuff), which, however, would be 
overkill for RELENG_4's single threaded kernel.

In RELENG_4's implementation of kevents, the only case in which 
KN_DETACHED gets set is when a process exits and posts a NOTE_EXIT 
event.  That is, the meaning of KN_DETACHED is much narrower than in 
RELENG_5.  For this reason I believe the most appropriate fix would be 
to check for KN_DETACHED in filt_sigdetach() in the same way it is 
already done in filt_procdetach().  In fact, if you compare the two 
routines it becomes pretty obvious that they should have been identical 
in the first place, and that the absence of said check from 
filt_sigdetach() is most likely just an oversight.

Therefore, I suggest to adopt the attached patch and leave the rest of 
RELENG_4's kevent code alone.  I checked the kernel sources and found 
that filt_procdetach() and filt_sigdetach() are in fact the only 
f_detach() routines that deal with a process' p_klist field, and 
therefore need this kind of safeguard.

Also, it would probably be a good idea to fix RELENG_4 swiftly (and 
possibly release a security advisory) because this flaw is certainly a 
great DoS opportunity for maliciously minded shell users ...

    Uwe
-- 
Uwe Doering         |  EscapeBox - Managed On-Demand UNIX Servers
gemini@geminix.org  |  http://www.escapebox.net
-------------- next part --------------
--- src/sys/kern/kern_sig.c.orig	Thu Feb  5 23:26:48 2004
+++ src/sys/kern/kern_sig.c	Sat Oct 23 11:15:30 2004
@@ -1739,6 +1739,10 @@
 {
 	struct proc *p = kn->kn_ptr.p_proc;
 
+	if (kn->kn_status & KN_DETACHED)
+		return;
+
+	/* XXX locking?  this might modify another process. */
 	SLIST_REMOVE(&p->p_klist, kn, knote, kn_selnext);
 }

Uwe Doering wrote:
> Igor Sysoev wrote:
> 
>> Here is more correct patch to fix the panic in 4.x reported in
>> http://freebsd.rambler.ru/bsdmail/freebsd-hackers_2004/msg02732.html
>>
>> -------------------------
>> --- src/sys/kern/kern_event.c   Sun Oct 10 12:17:55 2004
>> +++ src/sys/kern/kern_event.c   Sun Oct 10 12:19:29 2004
>> @@ -794,7 +794,8 @@
>>             while (kn != NULL) {
>>                 kn0 = SLIST_NEXT(kn, kn_link);
>>                 if (kq == kn->kn_kq) {
>> -                   kn->kn_fop->f_detach(kn);
>> +                   if (!(kn->kn_status & KN_DETACHED))
>> +                       kn->kn_fop->f_detach(kn);
>>         /* XXX non-fd release of kn->kn_ptr */
>>                     knote_free(kn);
>>                     *knp = kn0;
>> -------------------------
> 
> 
> Your patch appears to be an excerpt from the fix to RELENG_5.  May I 
> suggest a different approach for RELENG_4?  My reasoning is that the 
> implementation of kevents differs between RELENG_4 and RELENG_5.
> 
> In RELENG_5 the flag KN_DETACHED is used in a more general way.  It gets 
> set by knlist_add() and cleared by knlist_remove(), in sync with list 
> insertion and removal.
> [...]
Slight correction: The logic is of course the other way round.  That is, 
the text is supposed to read:

   It gets cleared by knlist_add() and set by knlist_remove() ...

Sorry about that. :-)

    Uwe
-- 
Uwe Doering         |  EscapeBox - Managed On-Demand UNIX Servers
gemini@geminix.org  |  http://www.escapebox.net

On Sat, 23 Oct 2004, Uwe Doering wrote:
> Igor Sysoev wrote:
> > Here is more correct patch to fix the panic in 4.x reported in
> > http://freebsd.rambler.ru/bsdmail/freebsd-hackers_2004/msg02732.html
> >
> > -------------------------
> > --- src/sys/kern/kern_event.c   Sun Oct 10 12:17:55 2004
> > +++ src/sys/kern/kern_event.c   Sun Oct 10 12:19:29 2004
> > @@ -794,7 +794,8 @@
> >             while (kn != NULL) {
> >                 kn0 = SLIST_NEXT(kn, kn_link);
> >                 if (kq == kn->kn_kq) {
> > -                   kn->kn_fop->f_detach(kn);
> > +                   if (!(kn->kn_status & KN_DETACHED))
> > +                       kn->kn_fop->f_detach(kn);
> >         /* XXX non-fd release of kn->kn_ptr */
> >                     knote_free(kn);
> >                     *knp = kn0;
> > -------------------------
>
> Your patch appears to be an excerpt from the fix to RELENG_5.  May I
> suggest a different approach for RELENG_4?  My reasoning is that the
> implementation of kevents differs between RELENG_4 and RELENG_5.
I agree with your patch. It is similar to my early patch, however,
I think that is more correctly to check (kn->kn_status & KN_DETACHED)
instead of !SLIST_EMPTY(&p->p_klist).
> Also, it would probably be a good idea to fix RELENG_4 swiftly (and
> possibly release a security advisory) because this flaw is certainly a
> great DoS opportunity for maliciously minded shell users ...
Yes.


Igor Sysoev
http://sysoev.ru/en/