Commit 198e0c9d005f7cc44872c722e24a34c10c83736a
Committed by
Greg Kroah-Hartman
Greg Kroah-Hartman
1 parent
81699fed3c
can: kvaser_usb: Don't free packets when tight on URBs
commit b442723fcec445fb0ae1104888dd22cd285e0a91 upstream. 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> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Showing 1 changed file with 4 additions and 6 deletions Side-by-side Diff
drivers/net/can/usb/kvaser_usb.c
| ... | ... | @@ -1294,12 +1294,14 @@ |
| 1294 | 1294 | if (!urb) { |
| 1295 | 1295 | netdev_err(netdev, "No memory left for URBs\n"); |
| 1296 | 1296 | stats->tx_dropped++; |
| 1297 | - goto nourbmem; | |
| 1297 | + dev_kfree_skb(skb); | |
| 1298 | + return NETDEV_TX_OK; | |
| 1298 | 1299 | } |
| 1299 | 1300 | |
| 1300 | 1301 | buf = kmalloc(sizeof(struct kvaser_msg), GFP_ATOMIC); |
| 1301 | 1302 | if (!buf) { |
| 1302 | 1303 | stats->tx_dropped++; |
| 1304 | + dev_kfree_skb(skb); | |
| 1303 | 1305 | goto nobufmem; |
| 1304 | 1306 | } |
| 1305 | 1307 | |
| ... | ... | @@ -1334,6 +1336,7 @@ |
| 1334 | 1336 | } |
| 1335 | 1337 | } |
| 1336 | 1338 | |
| 1339 | + /* This should never happen; it implies a flow control bug */ | |
| 1337 | 1340 | if (!context) { |
| 1338 | 1341 | netdev_warn(netdev, "cannot find free context\n"); |
| 1339 | 1342 | ret = NETDEV_TX_BUSY; |
| ... | ... | @@ -1364,9 +1367,6 @@ |
| 1364 | 1367 | if (unlikely(err)) { |
| 1365 | 1368 | can_free_echo_skb(netdev, context->echo_index); |
| 1366 | 1369 | |
| 1367 | - skb = NULL; /* set to NULL to avoid double free in | |
| 1368 | - * dev_kfree_skb(skb) */ | |
| 1369 | - | |
| 1370 | 1370 | atomic_dec(&priv->active_tx_urbs); |
| 1371 | 1371 | usb_unanchor_urb(urb); |
| 1372 | 1372 | |
| ... | ... | @@ -1388,8 +1388,6 @@ |
| 1388 | 1388 | kfree(buf); |
| 1389 | 1389 | nobufmem: |
| 1390 | 1390 | usb_free_urb(urb); |
| 1391 | -nourbmem: | |
| 1392 | - dev_kfree_skb(skb); | |
| 1393 | 1391 | return ret; |
| 1394 | 1392 | } |
| 1395 | 1393 |