netdev - Re: [PATCH v3] netpoll: Close race condition between poll_one_napi and napi

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] [day] [month] [year] [list]

Message-Id: <20150923.143312.404914990603153762.davem@davemloft.net>
Date:	Wed, 23 Sep 2015 14:33:12 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	nhorman@...hat.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v3] netpoll: Close race condition between poll_one_napi
 and napi_disable

From: Neil Horman <nhorman@...hat.com>
Date: Wed, 23 Sep 2015 14:57:58 -0400

> Drivers might call napi_disable while not holding the napi instance poll_lock.
> In those instances, its possible for a race condition to exist between
> poll_one_napi and napi_disable.  That is to say, poll_one_napi only tests the
> NAPI_STATE_SCHED bit to see if there is work to do during a poll, and as such
> the following may happen:
> 
> CPU0				CPU1
> ndo_tx_timeout			napi_poll_dev
>  napi_disable			 poll_one_napi
>   test_and_set_bit (ret 0)
> 				  test_bit (ret 1)
>    reset adapter		   napi_poll_routine
> 
> If the adapter gets a tx timeout without a napi instance scheduled, its possible
> for the adapter to think it has exclusive access to the hardware  (as the napi
> instance is now scheduled via the napi_disable call), while the netpoll code
> thinks there is simply work to do.  The result is parallel hardware access
> leading to corrupt data structures in the driver, and a crash.
> 
> Additionaly, there is another, more critical race between netpoll and
> napi_disable.  The disabled napi state is actually identical to the scheduled
> state for a given napi instance.  The implication being that, if a napi instance
> is disabled, a netconsole instance would see the napi state of the device as
> having been scheduled, and poll it, likely while the driver was dong something
> requiring exclusive access.  In the case above, its fairly clear that not having
> the rings in a state ready to be polled will cause any number of crashes.
> 
> The fix should be pretty easy.  netpoll uses its own bit to indicate that that
> the napi instance is in a state of being serviced by netpoll (NAPI_STATE_NPSVC).
> We can just gate disabling on that bit as well as the sched bit.  That should
> prevent netpoll from conducting a napi poll if we convert its set bit to a
> test_and_set_bit operation to provide mutual exclusion
> 
> Change notes:
> V2)
> 	Remove a trailing whtiespace
> 	Resubmit with proper subject prefix
> 
> V3)
> 	Clean up spacing nits
> 
> Signed-off-by: Neil Horman <nhorman@...driver.com>
> CC: "David S. Miller" <davem@...emloft.net>
> CC: jmaxwell@...hat.com
> Tested-by: jmaxwell@...hat.com

Applied, thanks Neil.
--
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