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.
> 
> I think I will test it in the next days.
> 
Turns out I had written pretty much exactly the same patch earlier this year.
I have no idea why I did not send it out.

Anyway, I (and Sashiko) think the patch is incomplete. It does not protect
cmd_buffer when handling raw events (while executing corsairpsu_raw_event).
That is a pre-existing issue, though. Not sure if that should be fixed in a
separate patch or with this one. Thoughts ?

I'll send a separate patch to fix the sign extension and the shifting of
negative values in corsairpsu_linear11_to_int().

Thanks,
Guenter