Re: [PATCH v4 2/2] hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Critical] Unsafe DMA Buffer Allocation / Cacheline Sharing in `struct powerz_priv`
--

commit eb68688b567b41c8eb7f2b911d80d14a178791ce
Author: Sanman Pradhan <psanman@juniper.net>

hwmon: (powerz) Fix missing usb_kill_urb() on signal interrupt

This commit fixes a bug where wait_for_completion_interruptible_timeout
returning a negative value due to a signal would bypass the usb_kill_urb
cleanup. It captures the return value into a long and handles both the
signal and timeout cases appropriately.

> diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
> index a75b941bd6e2f..96438f5f05d48 100644
> --- a/drivers/hwmon/powerz.c
> +++ b/drivers/hwmon/powerz.c

[ ... ]

> @@ -127,8 +128,14 @@ static int powerz_read_data(struct usb_device *udev, struct powerz_priv *priv)
>  	if (ret)
>  		return ret;
>  
> -	if (!wait_for_completion_interruptible_timeout
> -	    (&priv->completion, msecs_to_jiffies(5))) {
> +	rc = wait_for_completion_interruptible_timeout(&priv->completion,
> +						       msecs_to_jiffies(5));

This isn't a bug introduced by this patch, but could this completion logic
be vulnerable to memory corruption on architectures with larger cache
lines?

Looking at struct powerz_priv:

struct powerz_priv {
	char transfer_buffer[64];
	struct mutex mutex;
	struct completion completion;
	struct urb *urb;
	int status;
};

Since transfer_buffer[64] only occupies 64 bytes, could it share a
cacheline with the mutex and completion fields on architectures where
ARCH_DMA_MINALIGN is greater than 64?

When the USB subsystem unmaps the DMA buffer after a transfer, the DMA
layer might invalidate the entire cacheline. If the CPU concurrently
modifies the completion or mutex fields during wait queue operations
or locking, could those CPU writes be lost due to the invalidation?

Would it be safer to allocate the transfer buffer separately using a
dedicated allocation, or add adequate padding to ensure it occupies its
own cacheline?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260410002521.422645-1-sanman.pradhan@hpe.com?part=2