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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1546858970.3037.21.camel@suse.com>
Date:   Mon, 07 Jan 2019 12:02:50 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Marek Vasut <marex@...x.de>, 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 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,

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.

	Regards
		Oliver