netdev - Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG

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]

Date:	Sun, 20 Mar 2016 16:52:51 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	eric.dumazet@...il.com
Cc:	geert@...ux-m68k.org, woojung.huh@...rochip.com,
	UNGLinuxDriver@...rochip.com, linux@...ck-us.net,
	rjw@...ysocki.net, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

From: Eric Dumazet <eric.dumazet@...il.com>
Date: Sun, 20 Mar 2016 09:35:52 -0700

> On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
>> If CONFIG_PM=n:
>> 
>>     drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
>>     drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no member named ‘runtime_auto’
>> 
>> If PM is disabled, the runtime_auto flag is not available, but auto
>> suspend is not enabled anyway.  Hence protect the check for runtime_auto
>> by #ifdef CONFIG_PM to fix this.
>> 
>> Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
>> Reported-by: Guenter Roeck <linux@...ck-us.net>
>> Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>
>> ---
>> Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper to
>> include/linux/pm.h, which always return false if CONFIG_PM is disabled.
>> 
>> The only other user in non-core code (drivers/usb/core/sysfs.c) has a
>> big #ifdef CONFIG_PM check around all PM-related code.
>> 
>> Thoughts?
>> ---
>>  drivers/net/usb/lan78xx.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index d36d5ebf37f355f2..7b9ac47b2ecf9905 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -3271,7 +3271,9 @@ struct rtnl_link_stats64 *lan78xx_get_stats64(struct net_device *netdev,
>>  	 * periodic reading from HW will prevent from entering USB auto suspend.
>>  	 * if autosuspend is disabled, read from HW.
>>  	 */
>> +#ifdef CONFIG_PM
>>  	if (!dev->udev->dev.power.runtime_auto)
>> +#endif
>>  		lan78xx_update_stats(dev);
>>  
>>  	mutex_lock(&dev->stats.access_lock);
> 
> Note that a ndo_get_stat64() handler is not allowed to sleep,
> so the mutex_lock() is not wise...
> 
> Historically /proc/net/dev handler but also bonding ndo_get_stats() used
> RCU or a rwlock or a spinlock.
> 
> So a complete fix would need to get rid of this mutex as well.

This function is also buggy for another reason.  The driver needs to
capture the statistics when the runtime suspend happens.

Since it's much easier to find things wrong rather than right with
this new code, I've decided to completely revert the commit that added
lan78xx_get_stats64().

Once a correct version is implemented we can add it back.

Thanks.