netdev - Re: [RFC PATCH net 2/2] amd-xgbe: Avoid sleeping in 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]

Date:   Tue, 15 Oct 2019 15:41:49 +0000
From:   "Lendacky, Thomas" <Thomas.Lendacky@....com>
To:     James Morse <james.morse@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "Dave S . Miller" <davem@...emloft.net>
Subject: Re: [RFC PATCH net 2/2] amd-xgbe: Avoid sleeping in napi_disable()
 while holding a spinlock

On 10/15/19 8:49 AM, James Morse wrote:
> xgbe_powerdown() takes an irqsave spinlock, then calls napi_disable()
> via xgbe_napi_disable(). napi_disable() might call msleep().
> DEBUG_ATOMIC_SLEEP isn't happy about this:
> | BUG: sleeping function called from invalid context at ../net/core/dev.c:6332
> | in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 2831, name: bash
> | CPU: 3 PID: 2831 Comm: bash Tainted: G        W         5.4.0-rc3-00001-g9dbe793f263b #114
> | Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive) (DT)
> | Call trace:
> |  dump_backtrace+0x0/0x160
> |  show_stack+0x24/0x30
> |  dump_stack+0xb0/0xf8
> |  ___might_sleep+0x124/0x148
> |  __might_sleep+0x54/0x90
> |  napi_disable+0x48/0x140
> |  xgbe_napi_disable+0x64/0xc0
> |  xgbe_powerdown+0xb0/0x120
> |  xgbe_platform_suspend+0x34/0x80
> |  pm_generic_freeze+0x3c/0x58
> |  acpi_subsys_freeze+0x2c/0x38
> |  dpm_run_callback+0x3c/0x1e8
> |  __device_suspend+0x130/0x468
> |  dpm_suspend+0x114/0x388
> |  hibernation_snapshot+0xe8/0x378
> |  hibernate+0x18c/0x2f8
> 
> Move xgbe_napi_disable() outside the spin_lock()d region of
> xgbe_powerdown(). This matches its use in xgbe_stop() ... but this
> might only be safe because of the earlier call to xgbe_free_irqs().
> 
> Signed-off-by: James Morse <james.morse@....com>
> 
> ---
> RFC as I'm not familiar with this driver. I'm happy to test a better fix!
> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index bfba7effcf9f..a6e6c21e921f 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> @@ -1278,10 +1278,10 @@ int xgbe_powerdown(struct net_device *netdev, unsigned int caller)
>  	hw_if->powerdown_tx(pdata);
>  	hw_if->powerdown_rx(pdata);
>  
> -	xgbe_napi_disable(pdata, 0);
> -
>  	spin_unlock_irqrestore(&pdata->lock, flags);
>  
> +	xgbe_napi_disable(pdata, 0);
> +

As far as I can tell, I think this is safe. Whether inside or outside the
spinlock doesn't make a difference to the interrupt routine since it
doesn't acquire this lock. And the suspend/resume functions can't be
called at the same time.

Thanks,
Tom

>  	DBGPR("<--xgbe_powerdown\n");
>  
>  	return 0;
>