netdev - Re: [patch] net: avoid race between netpoll and network fast path

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]

Message-ID: <4715A1B6.5020300@oracle.com>
Date:	Tue, 16 Oct 2007 22:46:30 -0700
From:	Tina Yang <tina.yang@...cle.com>
To:	David Miller <davem@...emloft.net>
CC:	mpm@...enic.com, netdev@...r.kernel.org
Subject: Re: [patch] net: avoid race between netpoll and network fast path

David Miller wrote:
> From: Tina Yang <tina.yang@...cle.com>
> Date: Tue, 16 Oct 2007 20:45:04 -0700
> 
>> The current netpoll design and implementation has serveral race issues with the
>> network fast path that panics/hangs the system or causes interface timeout/reset
>> but the fix is likely to have impact on the overall system performance and could
>> involve a large number of drivers.  The proposal is to disable the problem code
>> for normal operations but only to enable it at the time of crash in case polling
>> is necessary.  Tests that have been done included the bug fix verification
>> as well as regression check on the netlog results in various crash modes.
>>
>> Signed-off-by: Tina Yang <tina.yang@...cle.com>
> 
> This is at best a kludge, and it's the wrong way to approach this problem.
> 
> Fix the bug, and fix it right.
> 
	
> If you disable that stretch of code, what you've done is make the
> netpoll code hang and/or drop console messages if the TX queue is full
> in the driver and the only way to liberate TX space is to call into
> ->poll().


	Isn't net_rx_action() calling ->poll() to free the TX space ?
	TX queue full can only be emptied when the device is done transmitting
	not because of netpoll ->poll() it.  The softirq (net_rx_action)
	is the purpose for such an event.  Netconsole messages will be
	dropped if the device can't keep up with it regardless of netpoll
	->poll() or not.  If no dropping can be tolerated, then the
	netpoll upper layer probably should be redesigned to buffer the data.

	The poll_list currently is in a per_cpu structure, not being
	protected globally that netpoll thread from any cpu can
	trash it.

> 
> You haven't shown the precise race that leads to corruption so that someone
> so motivated can guide you towards a more correct fix if you are not
> capable of implementing it properly on your own.


	The precise race is
	1) net_rx_action get the dev from poll_list
	2) at the same time, netpoll poll_napi() get a hold of the poll lock
	   and calls ->poll(), remove dev from the poll list
	3) after it finishes, net_rx_action get the poll lock, and calls
	   ->poll() the second time, and panic when trying to remove (again)
	   the dev from the poll list.
	and I had logged all the crash info from the crash scenes into the
	bug database.

	As Matt Mackall had acknowledged, the network fast path went to great
	length to reduce locking overhead, should that be undone because of
	netpoll if that's what it takes to fix it more correctly ?

	

	

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html