rwatson at FreeBSD.org
Tue Apr 21 22:25:41 UTC 2009
On Wed, 22 Apr 2009, Mikolaj Golub wrote:
> RW> There are several bugs here, one difficult to fix (lack of
> RW> refcounting), but also stuff like ifp being derived from an interface
> RW> number twice, but checked against NULL only the first time (line 85
> RW> checked for NULL, re-queried but no check line 88). Fixing the top
> RW> bit of the function to only query the ifp once and check it for NULL
> RW> then would be a good idea. More fundamentally, we do need to refcount
> RW> ifnets when used from the management path, which is not all that hard
> RW> a change, but preferably to try the easy way first given where we are
> RW> in the release cycle.
> I was thinking a bit on this problem (the same issue was reported in
> kern/132734) and eliminating double call of ifnet_byindex() was the first
> thing I did. But I decided then that the proper fix would be to wrap all
> critical code in sysctl_ifdata in IFNET_RLOCK/IFNET_RUNLOCK (the patch is
> attached). It looks like I am wrong and my idea about how the kernel works
> is oversimplified? :-) Unfortunately, I didn't manage to reproduce the panic
> in my environment so I was not able to do some experiments and tests.
The problem is that you can't hold IFNET_RLOCK() over the sysctl copyouts.
I'm preparing a patch for 8-CURRENT that will add a refcount to struct ifnet
to handle this case, but that isn't an MFC candidate for 7.2. If fixing the
return value checks for ifnet_byindex() avoids the insta-panic Mike's running
into, it's a better 7.2 change at this point. We can target the ifnet
refcount changes for 7.3.
Robert N M Watson
University of Cambridge
More information about the freebsd-stable