Date: Mon, 14 Nov 2005 20:38:16 +0200 From: Ruslan Ermilov <ru@FreeBSD.org> To: Bill Paul <wpaul@FreeBSD.org> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/net if.c if_arcsubr.c if_arp.h if_bridge.c if_ef.c if_ethersubr.c if_fddisubr.c if_fwsubr.c if_i Message-ID: <20051114183816.GX87446@ip.net.ua> In-Reply-To: <20051114153616.DE50A16A421@hub.freebsd.org> References: <20051114141420.GW87446@ip.net.ua> <20051114153616.DE50A16A421@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--S5Rg6oz6PtEXgjQf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 14, 2005 at 03:36:16PM +0000, Bill Paul wrote: > > On Sun, Nov 13, 2005 at 09:23:45PM +0000, Bill Paul wrote: > > > [...] > > >=20 > > > > sys/compat/ndis subr_ndis.c=20 > > >=20 > > > [...] > > >=20 > > > > - Store pointer to the link-level address right in "struct ifnet" > > > > rather than in ifindex_table[]; all (except one) accesses are > > > > through ifp anyway. IF_LLADDR() works faster, and all (except > > > > one) ifaddr_byindex() users were converted to use ifp->if_addr. > > > > =20 > > > > - Stop storing a (pointer to) Ethernet address in "struct arpcom", > > > > and drop the IFP2ENADDR() macro; all users have been converted > > > > to use IF_LLADDR() instead. > > >=20 [...] > > > More importantly, you broke it in > > > a very subtle way that only shows up as a kernel panic a runtime. > > >=20 > > Can you please give me more details about this panic? >=20 > I already fixed it, but to see it, take the previous version of > subr_ndis.c (with your change) and compile it on a 6.0 system, then > try loading an NDIS driver. The problem is that IF_LLADDR() exists > on earlier versions of FreeBSD, but using it on 6.0 leads to a > NULL pointer dereference in NdisReadNetworkAddress() at driver > load time. On 6.0, you have to continue using IFP2ENADDR() anyway > (until this change is merged). I can only assume that on 6.x, > using IF_LLADDR() in this circumstance references a pointer that > hasn't been initialized yet. >=20 Yes. I've been able to reproduce it on 7.0 too, independently. The problem is that (at least) this function accesses "struct ifnet" before it has been fully initialized by if_attach(), through e.g. a call to ether_ifattach(), where the first (AF_LINK) address is added to the interface address list. The effect of using IF_LLADDR() at this unfortunate time is that in all of 5.x, 6.x, and 7.0 it will dereference a null pointer. I've just committed a fix for this to HEAD after playing with an NDIS driver for RTL8139 pccard and confirming and fixing the problem. If it's possible to avoid using "struct ifnet" before it's if_attached()'ed and initialized (not sure), it would best. If not, there should be a branch-compatible way to tell if an address list has been initialized; then IF_LLADDR() can be used anywhere it's defined instead of IFP2ENADDR(). Moreover, I'm not sure, but maybe a check against an "empty" MAC address is redundant now, you should know better. ;) Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --S5Rg6oz6PtEXgjQf Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (FreeBSD) iD8DBQFDeNmYqRfpzJluFF4RAutDAJ9zhbjEUyTDr8IehIut6QfUNG8lmACfbEBG HfpPoQ9vbi2DDzeGpxDQk6w= =UKuX -----END PGP SIGNATURE----- --S5Rg6oz6PtEXgjQf--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051114183816.GX87446>