Date: Sat, 23 Oct 2004 15:57:27 +0200 From: Uwe Doering <gemini@geminix.org> To: freebsd-stable@freebsd.org Subject: Re: panic caused by EVFILT_SIGNAL detaching in rfork()ed thread Message-ID: <417A6347.8090207@geminix.org> In-Reply-To: <20041023003246.Y91215@is.park.rambler.ru> References: <20041023003246.Y91215@is.park.rambler.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------050609080300010801080308 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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 --------------050609080300010801080308 Content-Type: text/plain; name="kern_sig.c.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="kern_sig.c.diff" --- 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); } --------------050609080300010801080308--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?417A6347.8090207>