linux-kernel - Re: [PATCH v3 00/23] counter: cleanups and device lifetime fixes

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, 11 Jan 2022 11:56:26 +0200
From:   Jarkko Nikula <jarkko.nikula@...ux.intel.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        William Breathitt Gray <vilhelm.gray@...il.com>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH v3 00/23] counter: cleanups and device lifetime fixes

Hi

On 1/6/22 17:13, Uwe Kleine-König wrote:
> On Wed, Jan 05, 2022 at 09:26:58PM +0900, William Breathitt Gray wrote:
>> On Wed, Dec 29, 2021 at 04:44:18PM +0100, Uwe Kleine-König wrote:
>>>   - I think intel-qep.c makes the counter unfunctional in
>>>     intel_qep_remove before the counter is unregistered.
>>
>> Hello Uwe,
>>
>> Would you elaborate some more on this? I think intel_qep_remove() is
>> only called after the counter is unregistered because the struct
>> counter_device parent is set to &pci->dev in intel_qep_probe(). Am I
>> misunderstanding the removal path?
> 
> If the counter device is unbound (e.g. via sysfs), the following calls
> are made:
> 
> 	intel_qep_remove() (stopping the hardware?)
> 	devm_counter_release (devm callback of devm_counter_register or ..._add)
> 	then the release callbacks of the earlier devm functions
> 
> My concern is, that in the timeslot between intel_qep_remove() and
> devm_counter_release() the device looks like a functional device and
> might be queried/reconfigured/... while the hardware is already dead.
> 
> It's probably not a big issue (unless for example reading the counter
> this race window makes the hardware hang?), but it's at least ugly.
> Maybe the worst effect is that a counter value is missed (which is OK at
> unregister time). Still it would be nicer to first take down the counter
> device and only then stop the hardware.
> 
In HW point of view it should be safe. We do disable the HW in 
intel_qep_remove() but that doesn't render the HW unusable and registers 
are accessible.

Perhaps that line can go since I think it was put there just to stop the 
HW just in case after remove.

Jarkko