Skip to content

Commit

Permalink
can: kvaser_usb: Don't free packets when tight on URBs
Browse files Browse the repository at this point in the history
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.

On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.

Note:

Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.

This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.

Acked-by: Olivier Sobrie <olivier@sobrie.be>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
  • Loading branch information
Ahmed S. Darwish authored and Marc Kleine-Budde committed Jan 15, 2015
1 parent 47e3485 commit b442723
Showing 1 changed file with 4 additions and 6 deletions.
10 changes: 4 additions & 6 deletions drivers/net/can/usb/kvaser_usb.c
Original file line number Diff line number Diff line change
Expand Up @@ -1294,12 +1294,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
if (!urb) {
netdev_err(netdev, "No memory left for URBs\n");
stats->tx_dropped++;
goto nourbmem;
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}

buf = kmalloc(sizeof(struct kvaser_msg), GFP_ATOMIC);
if (!buf) {
stats->tx_dropped++;
dev_kfree_skb(skb);
goto nobufmem;
}

Expand Down Expand Up @@ -1334,6 +1336,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
}
}

/* This should never happen; it implies a flow control bug */
if (!context) {
netdev_warn(netdev, "cannot find free context\n");
ret = NETDEV_TX_BUSY;
Expand Down Expand Up @@ -1364,9 +1367,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
if (unlikely(err)) {
can_free_echo_skb(netdev, context->echo_index);

skb = NULL; /* set to NULL to avoid double free in
* dev_kfree_skb(skb) */

atomic_dec(&priv->active_tx_urbs);
usb_unanchor_urb(urb);

Expand All @@ -1388,8 +1388,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
kfree(buf);
nobufmem:
usb_free_urb(urb);
nourbmem:
dev_kfree_skb(skb);
return ret;
}

Expand Down

0 comments on commit b442723

Please sign in to comment.