Re: [PATCH v4 1/3] mfd: add support for Diolan DLN-2 devices

[Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Tue, Sep 09, 2014 at 10:24:44PM +0300, Octavian Purdila wrote:

> +static int alloc_rx_slot(struct dln2_mod_rx_slots *rxs)
> +{
> +	int ret;
> +	int slot;
> +
> +	/* No need to timeout here, the wait is bounded by the timeout
> +	 * in _dln2_transfer
> +	 */
> +	ret = wait_event_interruptible(rxs->wq, find_free_slot(rxs, &slot));
> +	if (ret < 0)
> +		return ret;
> +
> +	return slot;
> +}
> +
> +static void free_rx_slot(struct dln2_dev *dln2, struct dln2_mod_rx_slots *rxs,
> +			 int slot)
> +{
> +	struct urb *urb = NULL;
> +	unsigned long flags;
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int ret;
> +
> +	spin_lock_irqsave(&rxs->lock, flags);
> +
> +	clear_bit(slot, &rxs->bmap);
> +
> +	rxc = &rxs->slots[slot];
> +	rxc->connected = false;
> +	urb = rxc->urb;
> +	rxc->urb = NULL;
> +	reinit_completion(&rxc->done);
> +
> +	spin_unlock_irqrestore(&rxs->lock, flags);
> +
> +	if (urb)  {
> +		ret = usb_submit_urb(urb, GFP_KERNEL);
> +		if (ret < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", ret);
> +	}
> +
> +	wake_up_interruptible(&rxs->wq);
> +}

> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
> +			     u16 handle, u16 rx_slot)
> +{
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	int err;
> +
> +	spin_lock(&rxs->lock);
> +	rxc = &rxs->slots[rx_slot];
> +	if (rxc->connected) {
> +		rxc->urb = urb;
> +		complete(&rxc->done);
> +	} else {
> +		dev_warn(dev, "dropping response %d/%d", handle, rx_slot);
> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +	}
> +	spin_unlock(&rxs->lock);
> +}

> +static void dln2_rx(struct urb *urb)
> +{
> +	int err;
> +	struct dln2_dev *dln2 = urb->context;
> +	struct dln2_header *hdr = urb->transfer_buffer;
> +	struct device *dev = &dln2->interface->dev;
> +	u16 id, echo, handle, size;
> +	u8 *data;
> +	int len;
> +
> +	switch (urb->status) {
> +	case 0:
> +		/* success */
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +	case -EPIPE:
> +		/* this urb is terminated, clean up */
> +		dev_dbg(dev, "urb shutting down with status %d\n", urb->status);
> +		return;
> +	default:
> +		dev_dbg(dev, "nonzero urb status received %d\n", urb->status);
> +		goto out;
> +	}
> +
> +	if (urb->actual_length < sizeof(struct dln2_header)) {
> +		dev_err(dev, "short response: %d\n", urb->actual_length);
> +		goto out;
> +	}
> +
> +	handle = le16_to_cpu(hdr->handle);
> +	id = le16_to_cpu(hdr->id);
> +	echo = le16_to_cpu(hdr->echo);
> +	size = le16_to_cpu(hdr->size);
> +
> +	if (size != urb->actual_length) {
> +		dev_err(dev, "size mismatch: handle %x cmd %x echo %x size %d actual %d\n",
> +			handle, id, echo, size, urb->actual_length);
> +		goto out;
> +	}
> +
> +	if (handle >= DLN2_MAX_MODULES) {
> +		dev_warn(dev, "RX: invalid handle %d\n", handle);
> +		goto out;
> +	}
> +
> +	data = urb->transfer_buffer + sizeof(struct dln2_header);
> +	len = urb->actual_length - sizeof(struct dln2_header);
> +
> +	if (handle == DLN2_HANDLE_EVENT) {
> +		dln2_run_rx_callbacks(dln2, id, echo, data, len);
> +		err = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (err < 0)
> +			goto out_submit_failed;
> +	} else {
> +		dln2_rx_transfer(dln2, urb, handle, echo);
> +	}
> +
> +	return;
> +
> +out:
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +out_submit_failed:
> +	if (err < 0)
> +		dev_err(dev, "failed to re-submit RX URB: %d\n", err);
> +}

> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
> +			  const void *obuf, unsigned obuf_len,
> +			  void *ibuf, unsigned *ibuf_len)
> +{
> +	int ret = 0;
> +	u16 result;
> +	int rx_slot;
> +	struct dln2_response *rsp;
> +	struct dln2_rx_context *rxc;
> +	struct device *dev = &dln2->interface->dev;
> +	const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
> +	struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
> +
> +	rx_slot = alloc_rx_slot(rxs);
> +	if (rx_slot < 0)
> +		return rx_slot;
> +
> +	ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
> +	if (ret < 0) {
> +		free_rx_slot(dln2, rxs, rx_slot);
> +		dev_err(dev, "USB write failed: %d", ret);
> +		return ret;
> +	}
> +
> +	rxc = &rxs->slots[rx_slot];
> +
> +	ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		goto out_free_rx_slot;
> +	}
> +
> +	/* if we got here we know that the response header has been checked */
> +	rsp = rxc->urb->transfer_buffer;
> +	if (rsp->hdr.size < sizeof(*rsp)) {
> +		ret = -EPROTO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	result = le16_to_cpu(rsp->result);
> +	if (result) {
> +		dev_dbg(dev, "%d received response with error %d\n",
> +			handle, result);
> +		ret = -EREMOTEIO;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (!ibuf) {
> +		ret = 0;
> +		goto out_free_rx_slot;
> +	}
> +
> +	if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
> +		*ibuf_len = rsp->hdr.size - sizeof(*rsp);
> +
> +	memcpy(ibuf, rsp + 1, *ibuf_len);
> +
> +out_free_rx_slot:
> +	free_rx_slot(dln2, rxs, rx_slot);
> +
> +	return ret;
> +}
> +
> +int dln2_transfer(struct platform_device *pdev, u16 cmd,
> +		  const void *obuf, unsigned obuf_len,
> +		  void *ibuf, unsigned *ibuf_len)
> +{
> +	struct dln2_platform_data *dln2_pdata;
> +	struct dln2_dev *dln2;
> +	u16 h;
> +
> +	/* USB device has been disconnected, bail out */
> +	if (!pdev)
> +		return -ENODEV;

This isn't sufficient to prevent I/O after disconnect, or worse.

You set pdev to NULL in the platform device's remove callbacks, but you
have no synchronisation.

To take one example: in _dln2_transfer above you wait for completion,
but you never wake the process up in case the urb is killed due to a
disconnect. Hence you get a timeout much later, call free_rx_slot which
tries to send of another urb using potentially already freed data.

> +
> +	dln2 = dev_get_drvdata(pdev->dev.parent);
> +	dln2_pdata = dev_get_platdata(&pdev->dev);
> +	h = dln2_pdata->handle;
> +
> +	return _dln2_transfer(dln2, h, cmd, obuf, obuf_len, ibuf, ibuf_len);
> +}
> +EXPORT_SYMBOL(dln2_transfer);

> +static void dln2_free_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		usb_kill_urb(dln2->rx_urb[i]);
> +		usb_free_urb(dln2->rx_urb[i]);
> +		kfree(dln2->rx_buf[i]);
> +	}
> +}
> +
> +static void dln2_free(struct dln2_dev *dln2)
> +{
> +	dln2_free_rx_urbs(dln2);
> +	usb_put_dev(dln2->usb_dev);
> +	kfree(dln2);
> +}

> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	mfd_remove_devices(&interface->dev);

You need to make sure all I/O has stopped here.

> +	dln2_free(dln2);
> +}

Johan