Regarding if_alloc()

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]?


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=Ahu06i62sR8HDtDypao8Wcj9
tAcJ
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

At Thu, 17 Apr 2008 18:35:23 -0700 (PDT),
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]?
> 

It is almost always acceptable to submit a patch 

> 
> 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;
>     }
> 
> 

There are still parts of the network device infrastructure
that need
some locking, and it would seem that this is one of them.  I
know
Brooks Davis was also looking at this stuff so he may
comment as well.

Best,
George
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"

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.

-- 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=Ahu06i62sR8HDtDypao8Wcj9
tAcJ
> _______________________________________________
> freebsd-netfreebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"
> 

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=Ahu06i62sR8HDtDypao8Wcj9
tAcJ
>> _______________________________________________
>> freebsd-netfreebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"
>>
>
_______________________________________________
freebsd-netfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to
"freebsd-net-unsubscribefreebsd.org"