Re: [PATCH v2] watchdog: add driver for StreamLabs USB watchdog device

[Oliver Neukum <oneukum@suse.com>]

On Mon, 2016-04-18 at 03:53 +0100, Alexey Klimov wrote:
> This patch creates new driver that supports StreamLabs usb watchdog
> device. This device plugs into 9-pin usb header and connects to
> reset pin and reset button on common PC.
> 
> USB commands used to communicate with device were reverse
> engineered using usbmon.

Almost. I see only one issue.

> +struct streamlabs_wdt {
> +	struct watchdog_device wdt_dev;
> +	struct usb_interface *intf;
> +
> +	struct mutex lock;
> +	u8 buffer[BUFFER_LENGTH];

That is wrong.

> +};
> +

[..]

> +static int usb_streamlabs_wdt_command(struct watchdog_device *wdt_dev, u16 cmd)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
> +	struct usb_device *usbdev;
> +	int retval;
> +	int size;
> +	unsigned long timeout_msec;
> +
> +	int retry_counter = 10;		/* how many times to re-send stop cmd */
> +
> +	mutex_lock(&streamlabs_wdt->lock);
> +
> +	if (unlikely(!streamlabs_wdt->intf)) {
> +		mutex_unlock(&streamlabs_wdt->lock);
> +		return -ENODEV;
> +	}
> +
> +	usbdev = interface_to_usbdev(streamlabs_wdt->intf);
> +	timeout_msec = wdt_dev->timeout * MSEC_PER_SEC;
> +
> +	do {
> +		usb_streamlabs_wdt_prepare_buf((u16 *) streamlabs_wdt->buffer,
> +							cmd, timeout_msec);
> +		/* send command to watchdog */
> +		retval = usb_interrupt_msg(usbdev, usb_sndintpipe(usbdev, 0x02),
> +				streamlabs_wdt->buffer, BUFFER_TRANSFER_LENGTH,

Because of this line.

The problem is subtle. Your buffer and your lock share a cacheline.
On some architecture the cache is not consistent with respect to DMA.
On them cachelines holding a buffer for DMA need to be flushed to RAM
and invalidated and you may read from them only after DMA has finished.

Thus you may have prepared a cacheline for DMA but somebody tries taking
the lock. Then the cacheline with the lock is read from RAM. If that
happens before you finish the DMA the data resulting from DMA is lost.

The fix is to allocate the buffer with its own allocation. The VM
subsystem makes sure separate allocation don't share cachelines.

That is the long explanation for what I mean when I say that you violate
the DMA rules.

	Regards
		Oliver