Re: Kernel crash after using new Intel NIC (igb)

[Eric Dumazet <eric.dumazet@gmail.com>]

Le jeudi 26 mai 2011 à 12:30 -0700, Arun Sharma a écrit :
> On 5/24/11 11:35 PM, Eric Dumazet wrote:
> 
> >> Another possibility is to do the list_empty() check twice. Once without
> >> taking the lock and again with the spinlock held.
> >>
> >
> > Why ?
> >
> 
> Part of the problem is that I don't have a precise understanding of the 
> race condition that's causing the list to become corrupted.
> 
> All I know is that doing it under the lock fixes it. If it's slowing 
> things down, we do a check outside the lock (since it's cheap). But if 
> we get the wrong answer, we verify it again under the lock.
> 

You dont get the problem. Problem is : We can do the empty() test only
if protected by the lock.

If not locked, result can be wrong. [ false positive or negative ]


> > list_del_init(&p->unused); (done under lock of course) is safe, you can
> > call it twice, no problem.
> 
> Doing it twice is not a problem. But doing it when we shouldn't be doing 
> it could be the problem.
> 
> The list modification under unused_peers.lock looks generally safe. But 
> the control flow (based on refcnt) done outside the lock might have races.
> 

"might" is not a good word when dealing with this ;)

> Eg: inet_putpeer() might find the refcnt go to zero, but before it adds 
> it to the unused list, another thread may be doing inet_getpeer() and 
> set refcnt to 1. In the end, we end up with a node that's potentially in 
> use, but ends up on the unused list.
> 

Did you test my fix ?

Its doing the right thing : Using refcnt as the only marker to say if
the item must be removed from unused list (and lock the central lock
protecting this list only when needed)

Since we already must do an atomic operation on refcnt, using
atomic_inc_return [ or similar full barrier op ] is enough to tell us
the truth.