Regarding if_alloc()

[Brooks Davis]

On Tue, Apr 22, 2008 at 10:38:31AM -0700, vijay singh wrote:
> 
> 
> ----- Original Message ----
> From: Robert Watson <rwatson at FreeBSD.org>
> To: Brooks Davis <brooks at freebsd.org>
> Cc: vijay singh <vijjus at rocketmail.com>; freebsd-net at freebsd.org
> Sent: Sunday, April 20, 2008 2:27:15 AM
> Subject: Re: Regarding if_alloc()
> 
> 
> 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, I am working on the patch, but I had a few questions. If we
> drop the lock while if_grow() is running, how do we prevent a second
> caller, blocked in if_alloc() to start scanning the ifindex_table, and
> end up deciding to grow it again. In other words, we need to stall
> the ifindex_table scan in if_alloc() till if_grow() has achieved
> finished. Since if_grow() will be called relatively infrequently,
> should we consider holding the lock through the routine? Your comments
> and suggestions are welcome.

You can't hold locks over malloc calls.

-- Brooks
-------------- 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/freebsd-net/attachments/20080422/8c070b93/attachment.pgp