netdev - Re: [PATCH net v3] net: ethtool: do runtime PM outside RTNL

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: Wed, 3 Jan 2024 12:24:18 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Stanislaw Gruszka <stanislaw.gruszka@...ux.intel.com>,
 Jakub Kicinski <kuba@...nel.org>
Cc: Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org,
 Johannes Berg <johannes.berg@...el.com>, Marc MERLIN <marc@...lins.org>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH net v3] net: ethtool: do runtime PM outside RTNL

On 03.01.2024 11:30, Stanislaw Gruszka wrote:
> On Wed, Dec 06, 2023 at 08:44:48AM -0800, Jakub Kicinski wrote:
>> On Wed,  6 Dec 2023 11:39:32 +0100 Johannes Berg wrote:
>>> As reported by Marc MERLIN, at least one driver (igc) wants or
>>> needs to acquire the RTNL inside suspend/resume ops, which can
>>> be called from here in ethtool if runtime PM is enabled.
>>>
>>> Allow this by doing runtime PM transitions without the RTNL
>>> held. For the ioctl to have the same operations order, this
>>> required reworking the code to separately check validity and
>>> do the operation. For the netlink code, this now has to do
>>> the runtime_pm_put a bit later.
>>
>> I was really, really hoping that this would serve as a motivation
>> for Intel to sort out the igb/igc implementation. The flow AFAICT
>> is ndo_open() starts the NIC, the calls pm_sus, which shuts the NIC
>> back down immediately (!?) then it schedules a link check from a work
> 
> It's not like that. pm_runtime_put() in igc_open() does not disable device.
> It calls runtime_idle callback which check if there is link and if is
> not, schedule device suspend in 5 second, otherwise device stays running.
> 
> Work watchdog_task runs periodically and also check for link changes.
> 
>> queue, which opens it again (!?). It's a source of never ending bugs.
> 
> Maybe there are issues there and igc pm runtime implementation needs
> improvements, with lockings or otherwise. Some folks are looking at this. 
> But I think for this particular deadlock problem reverting of below commits
> should be considered:
> 
> bd869245a3dc net: core: try to runtime-resume detached device in __dev_open
> f32a21376573 ethtool: runtime-resume netdev parent before ethtool ioctl ops
> Reverting bd869245a3dc would break existing stuff.

> First, the deadlock should be addressed also in older kernels and
> refactoring is not really backportable fix.
> 
You could simply disable igc runtime pm on older kernel versions
if backporting a proper fix would be too cumbersome.

> Second, I don't think network stack should do any calls to pm_runtime* .

It's not unusual that subsystem core code deals with runtime pm.
E.g. see all the runtime pm calls in drivers/pci/pci.c
IMO it's exactly the purpose of the RPM API to encapsulate the
device-specific (r)pm features.

> This should be fully device driver specific, as this depends on device
> driver implementation of power saving. IMHO if it's desirable to 
> resume disabled device on requests from user space, then
> netif_device_detach() should not be used in runtime suspend.
> 
> Thoughts ?
> 
> Regards
> Stanislaw 
>