netdev - Hitting BUG_ON() from 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-next>] [day] [month] [year] [list]

Message-ID: <4EC16DE3.5020701@stratus.com>
Date:	Mon, 14 Nov 2011 14:37:07 -0500
From:	Mike McElroy <mike.mcelroy@...atus.com>
To:	netdev@...r.kernel.org
Subject: Hitting BUG_ON() from napi_enable in e1000e

Hitting the BUG_ON in napi_enable(). Code inspection shows that this can 
only be triggered by calling napi_enable() twice without an intervening 
napi_disable().

I saw the following sequence of events in the stack trace:

1) We simulated a cable pull using an Extreme switch.
2) e1000_tx_timeout() was entered.
3) e1000_reset_task() was called. Saw the message from e_err() in the 
console log.
4) e1000_reinit_locked was called. This function calls e1000_down() and 
e1000_up(). These functions call napi_disable() and napi_enable() 
respectively.
5) Then on another thread, a monitor task saw carrier was down and 
executed 'ip set link down' and 'ip set link up' commands.
6) Saw the '_E1000_RESETTING'warning fron the e1000_close function.
7) Either the e1000_open() executed between the e1000_down() and 
e1000_up() calls in step 4 or the e1000_open() call executed after the 
e0001_up() call. In either case, napi_enable() is called twice which 
triggers the BUG_ON.

This code sequence is present in the e1000 driver also.

There are two bugs here:
1) The napi_enable() and napi_disable() should only be called in the 
e1000_open and e1000_close functions respectively
2) There no synchronization preventing a call to the driver close while 
executing error processing.

Here is a patch for the napi_enable BUG_ON:

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 5ec1f99..e1af6fa 100755
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4242,9 +4242,6 @@ int e1000e_up(struct e1000_adapter *adapter)

         clear_bit(__E1000_DOWN, &adapter->state);

-#ifdef CONFIG_E1000E_NAPI
-       napi_enable(&adapter->napi);
-#endif
  #ifdef CONFIG_E1000E_MSIX
         if (adapter->msix_entries)
                 e1000_configure_msix(adapter);
@@ -4307,10 +4304,6 @@ void e1000e_down(struct e1000_adapter *adapter)
         /* flush both disables and wait for them to finish */
         e1e_flush();
         usleep_range(10000, 20000);
-
-#ifdef CONFIG_E1000E_NAPI
-       napi_disable(&adapter->napi);
-#endif
         e1000_irq_disable(adapter);

         del_timer_sync(&adapter->watchdog_timer);
@@ -4677,6 +4670,10 @@ static int e1000_close(struct net_device *netdev)

         pm_runtime_get_sync(&pdev->dev);

+#ifdef CONFIG_E1000E_NAPI
+       napi_disable(&adapter->napi);
+#endif
+
         if (!test_bit(__E1000_DOWN, &adapter->state)) {
                 e1000e_down(adapter);
                 e1000_free_irq(adapter);

--
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