Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Oliver Neukum <oneukum@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]



On 14.04.22 17:01, Andy Shevchenko wrote:
>
> Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
> before unregister_netdev()") which changed the ordering of the
> interface shutdown and basically makes this race happen? I don't see
> how we can guarantee that IOCTL won't be called until we quiescence
> the network device — my understanding that on device surprise removal
True. The best we could do is introduce a mutex for ioctl() and
disconnect(). That seems the least preferable solution to me.
> we have to first shutdown what it created and then unbind the device.
> If I understand the original issue correctly then the problem is in
> usbnet->unbind and it should actually be split to two hooks, otherwise
> it seems every possible IOCTL callback must have some kind of
> reference counting and keep an eye on the surprise removal.
>
> Johan, can you correct me if my understanding is wrong?
>
It seems to me that fundamentally the order of actions to handle
a hotunplug must mirror the order in a hotplug. We can add more hooks
if that turns out to be necessary for some drivers, but the basic
reverse mirrored order must be supported and I very much favor
restoring it as default.

So I am afraid I have to ask again, whether anybody sees a fundamental
issue with the attached patch, as opposed to it not being an elegant
solution?
It looks to me that we are in a fundamental disagreement on the correct
order in this question and there is no productive way forward other than
offering both ways.

    Regards
        Oliver

[-- Attachment #2: 0001-usbnet-split-unbind-callback.patch --]
[-- Type: text/x-patch, Size: 4357 bytes --]

From 2e07ccbd1769889963d129ec474909bdcaa4c64a Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 10 Mar 2022 13:18:38 +0100
Subject: [PATCH] usbnet: split unbind callback

Some devices need to be informed of a disconnect before
the generic layer is informed, others need their notification
later to avoid race conditions. Hence we provide two callbacks.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/asix_devices.c | 8 ++++----
 drivers/net/usb/smsc95xx.c     | 4 ++--
 drivers/net/usb/usbnet.c       | 7 +++++--
 include/linux/usb/usbnet.h     | 3 +++
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 6ea44e53713a..e6cfa9a39a87 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -808,7 +808,7 @@ static int ax88772_stop(struct usbnet *dev)
 	return 0;
 }
 
-static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 
@@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = {
 static const struct driver_info ax88772_info = {
 	.description = "ASIX AX88772 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1226,7 +1226,7 @@ static const struct driver_info ax88772_info = {
 static const struct driver_info ax88772b_info = {
 	.description = "ASIX AX88772B USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1262,7 +1262,7 @@ static const struct driver_info ax88178_info = {
 static const struct driver_info hg20f9_info = {
 	.description = "HG20F9 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 5567220e9d16..62db57021f5f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1211,7 +1211,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	return ret;
 }
 
-static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 
@@ -1985,7 +1985,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on)
 static const struct driver_info smsc95xx_info = {
 	.description	= "smsc95xx USB 2.0 Ethernet",
 	.bind		= smsc95xx_bind,
-	.unbind		= smsc95xx_unbind,
+	.unbind		= smsc95xx_disable,
 	.link_reset	= smsc95xx_link_reset,
 	.reset		= smsc95xx_reset,
 	.check_connect	= smsc95xx_start_phy,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index b1f93810a6f3..5249a7d7efa5 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1641,8 +1641,8 @@ void usbnet_disconnect (struct usb_interface *intf)
 		   xdev->bus->bus_name, xdev->devpath,
 		   dev->driver_info->description);
 
-	if (dev->driver_info->unbind)
-		dev->driver_info->unbind(dev, intf);
+	if (dev->driver_info->disable)
+		dev->driver_info->disable(dev, intf);
 
 	net = dev->net;
 	unregister_netdev (net);
@@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
+	if (dev->driver_info->unbind)
+		dev->driver_info->unbind (dev, intf);
+
 	usb_kill_urb(dev->interrupt);
 	usb_free_urb(dev->interrupt);
 	kfree(dev->padding_pkt);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8336e86ce606..4d2407f1ae93 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -129,6 +129,9 @@ struct driver_info {
 	/* cleanup device ... can sleep, but can't fail */
 	void	(*unbind)(struct usbnet *, struct usb_interface *);
 
+	/* disable device ... can sleep, but can't fail */
+	void	(*disable)(struct usbnet *, struct usb_interface *);
+
 	/* reset device ... can sleep */
 	int	(*reset)(struct usbnet *);
 
-- 
2.34.1