From owner-freebsd-stable@FreeBSD.ORG Sat Oct 23 13:57:31 2004 Return-Path: Delivered-To: freebsd-stable@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9751B16A4CE for ; Sat, 23 Oct 2004 13:57:31 +0000 (GMT) Received: from gen129.n001.c02.escapebox.net (gen129.n001.c02.escapebox.net [213.73.91.129]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1675D43D31 for ; Sat, 23 Oct 2004 13:57:31 +0000 (GMT) (envelope-from gemini@geminix.org) Message-ID: <417A6347.8090207@geminix.org> Date: Sat, 23 Oct 2004 15:57:27 +0200 From: Uwe Doering Organization: Private UNIX Site User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.2) Gecko/20041002 X-Accept-Language: en-us, en MIME-Version: 1.0 To: freebsd-stable@freebsd.org References: <20041023003246.Y91215@is.park.rambler.ru> In-Reply-To: <20041023003246.Y91215@is.park.rambler.ru> Content-Type: multipart/mixed; boundary="------------050609080300010801080308" Received: from gemini by geminix.org with asmtp (TLSv1:AES256-SHA:256) (Exim 3.36 #1) id 1CLMOf-000Jyb-00; Sat, 23 Oct 2004 15:57:29 +0200 Subject: Re: panic caused by EVFILT_SIGNAL detaching in rfork()ed thread X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Oct 2004 13:57:31 -0000 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--