From owner-cvs-src@FreeBSD.ORG Tue Sep 20 19:57:39 2005 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 89CB216A41F; Tue, 20 Sep 2005 19:57:39 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id F3AAB43D46; Tue, 20 Sep 2005 19:57:38 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (localhost.village.org [127.0.0.1] (may be forged)) by harmony.bsdimp.com (8.13.3/8.13.3) with ESMTP id j8KJuZ2p041289; Tue, 20 Sep 2005 13:56:35 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Tue, 20 Sep 2005 13:56:54 -0600 (MDT) Message-Id: <20050920.135654.73653698.imp@bsdimp.com> To: jhb@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <200509201551.11396.jhb@FreeBSD.org> References: <20050919055028.GC65954@ip.net.ua> <20050919.083146.105425670.imp@bsdimp.com> <200509201551.11396.jhb@FreeBSD.org> X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Tue, 20 Sep 2005 13:56:36 -0600 (MDT) Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, ru@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/an if_an.c src/sys/dev/arl if_arl_isa.c src/sys/dev/awi if_awi_pccard.c src/sys/dev/cm if_cm_isa.c src/sys/dev/cnw if_cnw.c src/sys/dev/cp if_cp.c src/sys/dev/cs if_cs.c src/sys/dev/ed if_ed.c src/sys/dev/em if_em.c ... X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Sep 2005 19:57:39 -0000 In message: <200509201551.11396.jhb@FreeBSD.org> John Baldwin writes: : On Monday 19 September 2005 10:31 am, M. Warner Losh wrote: : > In message: <20050919055028.GC65954@ip.net.ua> : > : > Ruslan Ermilov writes: : > : Hi, : > : : > : On Mon, Sep 19, 2005 at 03:10:21AM +0000, Warner Losh wrote: : > : > imp 2005-09-19 03:10:21 UTC : > : > : > : > FreeBSD src repository : > : > : > : > Modified files: : > : > sys/dev/an if_an.c : > : : > : [...] : > : : > : > sys/dev/xe if_xe_pccard.c : > : > Log: : > : > Make sure that we call if_free(ifp) after bus_teardown_intr. Since : > : > we could get an interrupt after we free the ifp, and the interrupt : > : > handler depended on the ifp being still alive, this could, in theory, : > : > cause a crash. Eliminate this possibility by moving the if_free to : > : > after the bus_teardown_intr() call. : > : : > : I'm going into more details in my other reply to your ed(4) commit, : > : but I'm pretty sure you're attacking a wrong problem here. Most : > : drivers don' assume and will behave badly (read: panic) if foo_intr() : > : is called after foo_stop() (foo_stop() disables interrupts and : > : usually frees some resources needed by code called from foo_intr()). : > : A better fix that I had in mind (and that I think jhb@ has suggested) : > : would be to return from foo_intr() quickly if IFF_DRV_RUNNING is not : > : set. Can you reproduce the problem that you mention on real hardware? : > : > You can't test IFF_DRV_RUNNING in memory that's already been freed. : : Then do this: : : foo_stop(); : callout_drain(); : ether_ifdetach(); : bus_teardowin_intr(); : if_free(); That's what my patches did :-) ruslan apparently thought I was fixing a different problem. : Regarding other comments I saw today on some e-mail or another, I do think : that to make the locking sane, we might should push the checks for : IFF_DRV_RUNNING down into the foo_start() routines rather than doing it in : the network layer where the driver lock isn't held. After playing around a bit, I'm convinced that we need to have the flags tested in the driver. While we might be able to do a dead_if sort of thing, driver checks, though tedious, are likely the way to go. Warner