linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: John Efstathiades <john.efstathiades@pebblebay.com>
To: unlisted-recipients:; (no To-header on input)
Cc: UNGLinuxDriver@microchip.com, woojung.huh@microchip.com,
	davem@davemloft.net, netdev@vger.kernel.org, kuba@kernel.org,
	linux-usb@vger.kernel.org, john.efstathiades@pebblebay.com
Subject: [PATCH net-next v2 09/10] lan78xx: Fix race condition in disconnect handling
Date: Tue, 24 Aug 2021 19:56:12 +0100	[thread overview]
Message-ID: <20210824185613.49545-10-john.efstathiades@pebblebay.com> (raw)
In-Reply-To: <20210824185613.49545-1-john.efstathiades@pebblebay.com>

If there is a device disconnect at roughly the same time as a
deferred PHY link reset there is a race condition that can result
in a kernel lock up due to a null pointer dereference in the
driver's deferred work handling routine lan78xx_delayedwork().
The following changes fix this problem.

Add new status flag EVENT_DEV_DISCONNECT to indicate when the
device has been removed and use it to prevent operations, such as
register access, that will fail once the device is removed.

Stop processing of deferred work items when the driver's USB
disconnect handler is invoked.

Disconnect the PHY only after the network device has been
unregistered and all delayed work has been cancelled.

Signed-off-by: John Efstathiades <john.efstathiades@pebblebay.com>
---
 drivers/net/usb/lan78xx.c | 66 +++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 9f395504f77e..4ec752d9751a 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -360,6 +360,7 @@ struct usb_context {
 #define EVENT_DEV_ASLEEP		7
 #define EVENT_DEV_OPEN			8
 #define EVENT_STAT_UPDATE		9
+#define EVENT_DEV_DISCONNECT		10
 
 struct statstage {
 	struct mutex			access_lock;	/* for stats access */
@@ -444,9 +445,13 @@ MODULE_PARM_DESC(msg_level, "Override default message level");
 
 static int lan78xx_read_reg(struct lan78xx_net *dev, u32 index, u32 *data)
 {
-	u32 *buf = kmalloc(sizeof(u32), GFP_KERNEL);
+	u32 *buf;
 	int ret;
 
+	if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
+		return -ENODEV;
+
+	buf = kmalloc(sizeof(u32), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -470,9 +475,13 @@ static int lan78xx_read_reg(struct lan78xx_net *dev, u32 index, u32 *data)
 
 static int lan78xx_write_reg(struct lan78xx_net *dev, u32 index, u32 data)
 {
-	u32 *buf = kmalloc(sizeof(u32), GFP_KERNEL);
+	u32 *buf;
 	int ret;
 
+	if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
+		return -ENODEV;
+
+	buf = kmalloc(sizeof(u32), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -3146,16 +3155,23 @@ static void tx_complete(struct urb *urb)
 		/* software-driven interface shutdown */
 		case -ECONNRESET:
 		case -ESHUTDOWN:
+			netif_dbg(dev, tx_err, dev->net,
+				  "tx err interface gone %d\n",
+				  entry->urb->status);
 			break;
 
 		case -EPROTO:
 		case -ETIME:
 		case -EILSEQ:
 			netif_stop_queue(dev->net);
+			netif_dbg(dev, tx_err, dev->net,
+				  "tx err queue stopped %d\n",
+				  entry->urb->status);
 			break;
 		default:
 			netif_dbg(dev, tx_err, dev->net,
-				  "tx err %d\n", entry->urb->status);
+				  "unknown tx err %d\n",
+				  entry->urb->status);
 			break;
 		}
 	}
@@ -3489,6 +3505,7 @@ static int rx_submit(struct lan78xx_net *dev, struct urb *urb, gfp_t flags)
 			lan78xx_defer_kevent(dev, EVENT_RX_HALT);
 			break;
 		case -ENODEV:
+		case -ENOENT:
 			netif_dbg(dev, ifdown, dev->net, "device gone\n");
 			netif_device_detach(dev->net);
 			break;
@@ -3689,6 +3706,12 @@ static void lan78xx_tx_bh(struct lan78xx_net *dev)
 		lan78xx_defer_kevent(dev, EVENT_TX_HALT);
 		usb_autopm_put_interface_async(dev->intf);
 		break;
+	case -ENODEV:
+	case -ENOENT:
+		netif_dbg(dev, tx_err, dev->net,
+			  "tx: submit urb err %d (disconnected?)", ret);
+		netif_device_detach(dev->net);
+		break;
 	default:
 		usb_autopm_put_interface_async(dev->intf);
 		netif_dbg(dev, tx_err, dev->net,
@@ -3783,6 +3806,9 @@ static void lan78xx_delayedwork(struct work_struct *work)
 
 	dev = container_of(work, struct lan78xx_net, wq.work);
 
+	if (test_bit(EVENT_DEV_DISCONNECT, &dev->flags))
+		return;
+
 	if (usb_autopm_get_interface(dev->intf) < 0)
 		return;
 
@@ -3857,6 +3883,7 @@ static void intr_complete(struct urb *urb)
 
 	/* software-driven interface shutdown */
 	case -ENOENT:			/* urb killed */
+	case -ENODEV:			/* hardware gone */
 	case -ESHUTDOWN:		/* hardware gone */
 		netif_dbg(dev, ifdown, dev->net,
 			  "intr shutdown, code %d\n", status);
@@ -3870,14 +3897,29 @@ static void intr_complete(struct urb *urb)
 		break;
 	}
 
-	if (!netif_running(dev->net))
+	if (!netif_device_present(dev->net) ||
+	    !netif_running(dev->net)) {
+		netdev_warn(dev->net, "not submitting new status URB");
 		return;
+	}
 
 	memset(urb->transfer_buffer, 0, urb->transfer_buffer_length);
 	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status != 0)
+
+	switch (status) {
+	case  0:
+		break;
+	case -ENODEV:
+	case -ENOENT:
+		netif_dbg(dev, timer, dev->net,
+			  "intr resubmit %d (disconnect?)", status);
+		netif_device_detach(dev->net);
+		break;
+	default:
 		netif_err(dev, timer, dev->net,
 			  "intr resubmit --> %d\n", status);
+		break;
+	}
 }
 
 static void lan78xx_disconnect(struct usb_interface *intf)
@@ -3892,8 +3934,15 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 	if (!dev)
 		return;
 
+	set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
+
 	udev = interface_to_usbdev(intf);
 	net = dev->net;
+
+	unregister_netdev(net);
+
+	cancel_delayed_work_sync(&dev->wq);
+
 	phydev = net->phydev;
 
 	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
@@ -3904,12 +3953,11 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 	if (phy_is_pseudo_fixed_link(phydev))
 		fixed_phy_unregister(phydev);
 
-	unregister_netdev(net);
-
-	cancel_delayed_work_sync(&dev->wq);
-
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
+	if (timer_pending(&dev->stat_monitor))
+		del_timer_sync(&dev->stat_monitor);
+
 	lan78xx_unbind(dev, intf);
 
 	usb_kill_urb(dev->urb_intr);
-- 
2.25.1


  parent reply	other threads:[~2021-08-24 18:57 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 18:56 [PATCH net-next v2 00/10] LAN7800 driver improvements John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 01/10] lan78xx: Fix white space and style issues John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 02/10] lan78xx: Remove unused timer John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 03/10] lan78xx: Set flow control threshold to prevent packet loss John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 04/10] lan78xx: Remove unused pause frame queue John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 05/10] lan78xx: Add missing return code checks John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 06/10] lan78xx: Fix exception on link speed change John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 07/10] lan78xx: Fix partial packet errors on suspend/resume John Efstathiades
2021-08-24 18:56 ` [PATCH net-next v2 08/10] lan78xx: Fix race conditions in suspend/resume handling John Efstathiades
2021-08-24 18:56 ` John Efstathiades [this message]
2021-08-24 18:56 ` [PATCH net-next v2 10/10] lan78xx: Limit number of driver warning messages John Efstathiades
2021-08-25 10:00 ` [PATCH net-next v2 00/10] LAN7800 driver improvements patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210824185613.49545-10-john.efstathiades@pebblebay.com \
    --to=john.efstathiades@pebblebay.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=woojung.huh@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).