Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver unsafely shares a single buffer (`priv->cmd_buffer`) for both transmitting commands and receiving asynchronous HID events, leading to a critical data race.
- [Medium] The patch introduces a custom mutex instead of using the hwmon subsystem's mandated locking API, violating subsystem guidelines and creating redundant locking.
- [Low] The newly introduced mutex is initialized but never destroyed, potentially leaking lockdep tracking state on module removal.
--

commit e6fd3791fd5a9cb958361af0e5c07256d1d52355
Author: Wilken Gottwalt <wilken.gottwalt@posteo.net>

hwmon: corsair-psu: fix and readd locking of command buffer

This commit re-adds a locking mechanism to protect chained commands from
parallel access by hwmon and debugfs subsystems. It introduces a custom
mutex in the corsairpsu_data structure to serialize these operations.

> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index dddbd2463f8da..f1e59a8debb8f 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
[ ... ]
> @@ -122,6 +123,7 @@ struct corsairpsu_data {
>  	struct device *hwmon_dev;
>  	struct dentry *debugfs;
>  	struct completion wait_completion;
> +	struct mutex lock; /* serializes chained commands and parallel debugfs/hwmon access */
>  	u8 *cmd_buffer;

This isn't a bug introduced by this patch, but could sharing a single
cmd_buffer for both transmitting and receiving cause a data race?

If a delayed reply from a previous command arrives right after
reinit_completion() in corsairpsu_usb_cmd(), the async event handler might
overwrite cmd_buffer. Because hid_hw_output_report() executes right after
reinit_completion(), could this result in transmitting a corrupted buffer
to the hardware?

>  	char vendor[REPLY_SIZE];
>  	char product[REPLY_SIZE];
[ ... ]
> @@ -217,6 +221,7 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
>  {
>  	int ret;
>  
> +	mutex_lock(&priv->lock);
>  	switch (cmd) {
>  	case PSU_CMD_RAIL_VOLTS_HCRIT:
>  	case PSU_CMD_RAIL_VOLTS_LCRIT:

The hwmon subsystem guidelines state that drivers should use hwmon_lock() and
hwmon_unlock() to implement locking required for interrupt handling or for
attributes registered by other means.

Should this use the subsystem-provided hwmon_lock(priv->hwmon_dev) instead
of introducing a custom mutex to protect against concurrent debugfs access?

[ ... ]
> @@ -789,6 +798,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
>  
>  	priv->hdev = hdev;
>  	hid_set_drvdata(hdev, priv);
> +	mutex_init(&priv->lock);
>  	init_completion(&priv->wait_completion);
>  
>  	hid_device_io_start(hdev);

If the custom mutex is kept, could initializing it with mutex_init() leak
lockdep tracking state when the module is removed?

Since the device data is allocated with devm_kzalloc(), would it be safer
to use devm_mutex_init() to ensure the lock is properly destroyed during
cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/agR9YW7hGTJ_l7ms@monster.localdomain?part=1