netdev - Re: 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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 15 Nov 2011 23:32:28 -0800
From:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:	Mike McElroy <mike.mcelroy@...atus.com>
Cc:	netdev@...r.kernel.org
Subject: Re: Hitting BUG_ON() from napi_enable in e1000e

On Mon, Nov 14, 2011 at 11:37, Mike McElroy <mike.mcelroy@...atus.com> wrote:
>
> 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);
>

Thanks, I will add this patch to my queue.

-- 
Cheers,
Jeff
--
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