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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
