Re: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon

On 8/2/26 05:57, Wilken Gottwalt wrote:
> On Sun,  2 Aug 2026 12:36:53 +0000
> Ali Ahmet Memis <[email protected]> wrote:
> 
>> corsairpsu_request() sends a rail select command and then the actual
>> read as two separate transfers, both going through the single shared
>> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
>> serializes its own callers, but the debugfs files call
>> corsairpsu_get_value() directly and never take that lock, so a debugfs
>> read can land between another reader's rail select and its value read.
>>
>> The result is a value from the wrong rail reported as the right one,
>> because corsairpsu_usb_cmd() only checks the command echo and both
>> transfers echo the command it expects. It can also make a caller consume
>> the reply meant for the other one, since raw_event() writes into the
>> shared buffer and completes whoever happens to be waiting.
>>
>> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
>> on subsystem locking") on the grounds that the subsystem serializes for
>> us, which holds for sysfs but not for these files. Take
>> the same lock in the debugfs paths that issue commands, using the guard
>> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
>> for subsystem locks"), as suggested in [1].
>>
>> The lock cannot go into corsairpsu_request() itself: the hwmon core
>> already holds it across ->read, so every sysfs read would deadlock.
>> vendor_show() and product_show() only print strings cached during probe
>> and issue no command, and corsairpsu_get_criticals() and
>> corsairpsu_check_cmd_support() run before either interface is
>> registered, so none of them need it.
>>
>> [1] https://lore.kernel.org/all/[email protected]/
>>
>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
>> Signed-off-by: Ali Ahmet Memis <[email protected]>
>> ---
>> This is the fix Guenter asked for in the May thread, written the way he
>> suggested there. Wilken's patch used a driver private mutex around
>> corsairpsu_request(); that thread stalled and the race is still present.
>>
>> Wilken, does this cover the chained command case you were worried about?
>> As far as I can tell it does: the whole select-rail plus read sequence
>> now runs under the same lock the hwmon core takes around ->read, so a
>> debugfs reader cannot land in the middle of one. If you had a case in
>> mind that this misses, I would rather hear it than guess.
> 
> Yes, I think that is what Guenter asked me to test. There is actually a
> way to get all values from the PSU at once. You can chain together all
> the commands and everything supported should even fit into a single USB
> HID frame (64bytes). That would make everything a bit easier. Though,
> sorry that I did not go on with that. About a day after this someone put
> basically all my open source projects through an AI agent and since then
> I get bombarded with AI slop. I currently have not much energy (and fun)
> left doing my projects.
> 

Welcome to the club. I currently have more than 100 bugs reported by Sashiko
open against the hardware monitoring subsystem, and more than 50 against the
watchdog subsystem. I try to fix the critical issues as well as the security
issues, but trying to fix all of them would be futile (and Sashiko would
probably end up finding more bugs when analyzing the fixes). And, then,
yes, people submit more and more fixes for cosmetic issues reported and/or
suggested by some AI.

> I think I will test it in the next days.

Please let me know if you can find the time (or not). In most cases like this
I end up trusting the Sashiko review.

Thanks,
Guenter