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

[Ruslan Ermilov]

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:
> > > [...]
> > > 
> > > >     sys/compat/ndis      subr_ndis.c 
> > > 
> > > [...]
> > > 
> > > >   - 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.
> > > >   
> > > >   - 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.
> > > 
[...]
> > > More importantly, you broke it in
> > > a very subtle way that only shows up as a kernel panic a runtime.
> > > 
> > Can you please give me more details about this panic?
> 
> 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.
> 
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,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20051114/584d9d3e/attachment.bin