On Fri, 18 Apr 2008, Brooks Davis wrote:
On Thu, Apr 17, 2008 at 06:35:23PM -0700, vijay singh wrote:
Hi all. How do we avoid a race in populating the ifindex_table? Id this is
a TODO, as it seems from the code below, would it be acceptable if I wrote
a patch and reused the ifnet_lock [IFNET_WLOCK, IFNET_WUNLOCK]?
Locking if_index management with ifnet_lock should be ok. Ideally we should
probably be using ALLOC_UNR(9) to manage if_indexes instead of this rather
expensive loop.
Be aware, that if_index generation is least of the issues in this area. The
if_grow() call is much riskier since it changes the value of the global
ifnet pointer which I'm not sure we can afford to lock. It would be worth
experimenting with rmlocks to see what the impact if of locking would be.
I'm serious tempted to kill if_grow in favor of some sort of if_index_max
tunable.
I've seen a number of reports of panics that may well be traceable to races in
if_index handling, and have looked a bit at possible fixes. Quite a few uses
of if_index are inherently racy, as they rely on stability of the index value,
which of course can't be guaranteed with removable interfaces.
I think a reasonable interim fix would be to protect all use of the byindex
arrays using the ifnet lock, but remember that the read methods, not just the
write methods, need protection, and as such should move from being macros in
if_var.h to functions in if.c. if_grow is probably OK if this is done right,
but it will need to be set up to drop the lock, grow, re-aquire, and
re-validate assumptions (i.e., repeat the search for a free index and loop if
it fails to find one). Once the read methods are using the lock also, we
should seriously consider converting it to an rwlock. We can probably also
un-publicize at least one of the byindex lookup routines (the dev lookup,
which is needed only in if.c).
This would prevent races on modifying and evaluating the index array, but not
on disappearing cdevs and ifnets, which are a separate problem, and one that
probably is exercised significanty less. The reports I've seen appear to have
only to do with pulling the array out from other consumers while in use.
Robert N M Watson
Computer Laboratory
University of Cambridge
-- Brooks
if_alloc(u_char type)
{
struct ifnet *ifp;
ifp = malloc(sizeof(struct ifnet), M_IFNET, M_WAITOK|M_ZERO);
/*
* Try to find an empty slot below if_index. If we fail, take
* the next slot.
*
* XXX: should be locked!
*/
for (ifp->if_index = 1; ifp->if_index <= if_index; ifp->if_index++) {
if (ifnet_byindex(ifp->if_index) == NULL)
break;
}
____________________________________________________________________________________
Be a better friend, newshound, and
know-it-all with Yahoo! Mobile. Try it now.
http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
