netdev - Re: [PATCH 02/19] usbnet: smsc95xx: Stop propagation of in

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: <bca17456-47e0-1935-b6ba-6a52331756ca@denx.de>
Date:   Mon, 7 Jan 2019 12:50:23 +0100
From:   Marek Vasut <marex@...x.de>
To:     Oliver Neukum <oneukum@...e.com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Nisar Sayed <Nisar.Sayed@...rochip.com>,
        Woojung Huh <Woojung.Huh@...rochip.com>,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH 02/19] usbnet: smsc95xx: Stop propagation of in_pm

On 1/7/19 12:02 PM, Oliver Neukum wrote:
> On Do, 2019-01-03 at 02:10 +0100, Marek Vasut wrote:
>> The information whether the SMSC95xx is in_pm or not can be derived from
>> the usbdev->suspend_count. First thing called in smsc95xx_suspend() is
>> usbnet_suspend(), which increments the usbdev->suspend_count and since
>> then the driver only calls _nopm() functions and functions with in_pm
>> set to 1. The smsc95xx_resume() does the inverse, it calls functions
>> with _nopm() suffix and with in_pm set to 1 until usbnet_resume(), which
>> sets the usbdev->suspend_count to 0.
> 
> Hi,

Hello Oliver,

> unfortunately I am forced to disagree in the strongest terms.
> 
> The logic behind this patch is wrong. The "in_pm" parameter
> exists because some function need to be called in the code paths
> needed to implement power management. Under those circumstances
> they must not take a pm reference to keep the device awake, lest
> the driver deadlock.
> (For example __smsc95xx_read_reg)
> 
> However from other code paths precisely that reference must be taken
> in order to make sure that the driver do not try to communicate with
> a suspended device.
> If you check the suspend counter, you will omit the necessary getting
> of a reference precisely when it is needed. The driver will never
> dedalock, but you will cause numerous races.
> 
> I must suggest to simply drop this part and redo the series.

OK, let's drop the whole series.

-- 
Best regards,
Marek Vasut