Date: Thu, 15 Sep 2005 23:56:39 +0300 From: Ruslan Ermilov <ru@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/re if_re.c Message-ID: <20050915205639.GD88456@ip.net.ua> In-Reply-To: <200509151521.14204.jhb@FreeBSD.org> References: <200509151859.j8FIxY6v007639@repoman.freebsd.org> <200509151521.14204.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--/unnNtmY43mpUSKx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 15, 2005 at 03:21:12PM -0400, John Baldwin wrote: > On Thursday 15 September 2005 02:59 pm, Ruslan Ermilov wrote: > > ru 2005-09-15 18:59:34 UTC > > > > FreeBSD src repository > > > > Modified files: > > sys/dev/re if_re.c > > Log: > > re_detach() fixes: > > > > - Fixed if_free() logic screw-up that can either result > > in freeing a NULL pointer or leaking "struct ifnet". > > - Move if_free() after re_stop(); the latter accesses > > "struct ifnet". This bug was masked by a previous bug. > > - Restore the fix for a panic on detach caused by racing > > with BPF detach code by Bill by moving ether_ifdetach() > > after re_stop() and resetting IFF_UP; this got screwed > > up in revs. 1.30 and 1.36. >=20 > Device drivers should not modify IFF_UP. Instead, the interrupt handler= =20 > should be checking IFF_DRV_RUNNING rather than IFF_UP (foo_stop() clears= =20 > IFF_DRV_RUNNING). >=20 Ideally they shoudn't, yes. But there are at least two scenarios where resetting IFF_UP is currently used to attack bugs. The first is the BPF detach bad interaction with foo_detach(), as described in re_detach(). This panic is real with (I think) all drivers. And testing IFF_DRV_RUNNING here doesn't seem to be able to prevent the panic. Perhaps the fix would be to move ether_ifdetach() before foo_stop() in foo_detach(), I'm not yet sure. Another panic sometimes seen on SMP machines on shutdown is when foo_intr() is called after foo_shutdown() has already been called. Some drivers (if_re, if_vr, if_ath) attack this problem by clearing IFF_UP so that foo_intr() bails out quickly. I don't know if anybody diagnosed the roots of this problem, but I have the following idea: foo_shutdown() calls foo_stop() which disables device interrupts. After foo_stop() has freed resources but before interrupt has been teared down, another device that shares the same IRQ generated an interrupt, and foo_intr() is called again. It checks ISR register, and it has some interrupt active, and the code calls some function that accesses already freed memory. I don't have any real SMP hardware to play with, so the above is pure theoretical. Do you think this is possible? If so, checking IFF_DRV_RUNNING in foo_intr() should fix this. If we can also fix the "BPF detach MII tick reactivation" panic without involving resetting IFF_UP, all drivers can be cleaned up to not much with IFF_UP. Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --/unnNtmY43mpUSKx Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (FreeBSD) iD8DBQFDKeAGqRfpzJluFF4RAqU9AJwMvHZoD2tN1+oKuRn0xglCqK4JaACfYQ10 rmr8m41YHBHtZfIysdpmQAY= =9Isy -----END PGP SIGNATURE----- --/unnNtmY43mpUSKx--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050915205639.GD88456>