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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 26 May 2011 21:47:26 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Arun Sharma <asharma@...com>
Cc:	Maximilian Engelhardt <maxi@...monizer.de>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	StuStaNet Vorstand <vorstand@...sta.mhn.de>
Subject: Re: Kernel crash after using new Intel NIC (igb)

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/