| Message ID | 1542977486-15363-1-git-send-email-3ernd.Eckstein@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 45611c61dd503454b2edae00aabe1e429ec49ebe |
| Headers | show |
| Series | usbnet: ipheth: fix potential recvmsg bug and recvmsg bug 2 | expand |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On Fri, 2018-11-23 at 13:51 +0100, Bernd Eckstein wrote: > This causes the following problems: > - double free of the skb or potential memory leak > - in dmesg: 'recvmsg bug' and 'recvmsg bug 2' and eventually > general protection fault For what it's worth, Bernd contacted me because it looks like this was the cause of https://lore.kernel.org/lkml/20180605085450.GA3506@scapa.corsac.net/ I wanted to try Bernd patch, unfortunately I wasn't able to reproduce the freezes on more recent kernels, so right now I can't add a Tested-By, but I'd be happy to be kept on CC. Regards, - -- Yves-Alexis -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAlv3+4YACgkQ3rYcyPpX RFuuoQgAhJHvzpl1yr7ZM215DviNj8Q/n7WpiWSk6ik5Ts8n4qsgIRcXojhEPfkw KAmpJ7G1zRgxRtZayiKLUl7RE3uWNOjTph3pzztBmMI+P7C/R4pVdZeRlandGsju YH6G32u+BB55tJw/TYrahlG3Ni0HNDoL+QfgJJno6/dr9V+YFr+opTCuDchaBHeJ acVsogUkyclvAutHt3UMjiDvVB12mW6IFz997y1Fv3JMT9HTQrDT9VdMUSr2WC5s rb9ONoim1ADDgTBN10mbPWZ80aooR8GtNEGpFa2EL0vPdNVlqyZqV46hYbLuZZ42 tMF3AD6hrQqxitCS3QGJpWjFFscFMA== =OkrF -----END PGP SIGNATURE-----
From: Bernd Eckstein <3erndeckstein@gmail.com> Date: Fri, 23 Nov 2018 13:51:26 +0100 > The bug is not easily reproducable, as it may occur very infrequently > (we had machines with 20minutes heavy downloading before it occurred) > However, on a virual machine (VMWare on Windows 10 host) it occurred > pretty frequently (1-2 seconds after a speedtest was started) > > dev->tx_skb mab be freed via dev_kfree_skb_irq on a callback > before it is set. > > This causes the following problems: > - double free of the skb or potential memory leak > - in dmesg: 'recvmsg bug' and 'recvmsg bug 2' and eventually > general protection fault > > Example dmesg output: ... > The proposed patch eliminates a potential racing condition. > Before, usb_submit_urb was called and _after_ that, the skb was attached > (dev->tx_skb). So, on a callback it was possible, however unlikely that the > skb was freed before it was set. That way (because dev->tx_skb was not set > to NULL after it was freed), it could happen that a skb from a earlier > transmission was freed a second time (and the skb we should have freed did > not get freed at all) > > Now we free the skb directly in ipheth_tx(). It is not passed to the > callback anymore, eliminating the posibility of a double free of the same > skb. Depending on the retval of usb_submit_urb() we use dev_kfree_skb_any() > respectively dev_consume_skb_any() to free the skb. > > Signed-off-by: Oliver Zweigle <Oliver.Zweigle@faro.com> > Signed-off-by: Bernd Eckstein <3ernd.Eckstein@gmail.com> Applied and queued up for -stable, thanks.
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 7275761..3d8a70d 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -140,7 +140,6 @@ struct ipheth_device { struct usb_device *udev; struct usb_interface *intf; struct net_device *net; - struct sk_buff *tx_skb; struct urb *tx_urb; struct urb *rx_urb; unsigned char *tx_buf; @@ -230,6 +229,7 @@ static void ipheth_rcvbulk_callback(struct urb *urb) case -ENOENT: case -ECONNRESET: case -ESHUTDOWN: + case -EPROTO: return; case 0: break; @@ -281,7 +281,6 @@ static void ipheth_sndbulk_callback(struct urb *urb) dev_err(&dev->intf->dev, "%s: urb status: %d\n", __func__, status); - dev_kfree_skb_irq(dev->tx_skb); if (status == 0) netif_wake_queue(dev->net); else @@ -423,7 +422,7 @@ static int ipheth_tx(struct sk_buff *skb, struct net_device *net) if (skb->len > IPHETH_BUF_SIZE) { WARN(1, "%s: skb too large: %d bytes\n", __func__, skb->len); dev->net->stats.tx_dropped++; - dev_kfree_skb_irq(skb); + dev_kfree_skb_any(skb); return NETDEV_TX_OK; } @@ -443,12 +442,11 @@ static int ipheth_tx(struct sk_buff *skb, struct net_device *net) dev_err(&dev->intf->dev, "%s: usb_submit_urb: %d\n", __func__, retval); dev->net->stats.tx_errors++; - dev_kfree_skb_irq(skb); + dev_kfree_skb_any(skb); } else { - dev->tx_skb = skb; - dev->net->stats.tx_packets++; dev->net->stats.tx_bytes += skb->len; + dev_consume_skb_any(skb); netif_stop_queue(net); }