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

Oliver Neukum
2022-04-21T11:19:02Z

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

So I am afraid I have to ask again, whether anybody sees a fundamental
issue with the new version attached patch, as opposed to it not being an
elegant
solution?

I corrected the stuff Johan found and split the method in the asix driver.

I do not understand smsc95xx well, so I left it in the current state.

Regards
Oliver

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

From 5953b3b12dd6cdd8d0bdb0119ee627d62219ab1e 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 | 13 +++++++++++--
drivers/net/usb/smsc95xx.c | 4 ++--
drivers/net/usb/usbnet.c | 10 +++++++---
include/linux/usb/usbnet.h | 3 +++
4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 38e47a93fb83..cde4d234b975 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -804,12 +804,18 @@ 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;

phy_disconnect(priv->phydev);
- asix_rx_fixup_common_free(dev->driver_priv);
+}
+
+static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+ struct asix_common_private *priv = dev->driver_priv;
+
+ asix_rx_fixup_common_free(priv);
}

static void ax88178_unbind(struct usbnet *dev, struct usb_interface *intf)
@@ -1211,6 +1217,7 @@ static const struct driver_info ax88772_info = {
.description = "ASIX AX88772 USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = ax88772_disable,
.status = asix_status,
.reset = ax88772_reset,
.stop = ax88772_stop,
@@ -1223,6 +1230,7 @@ static const struct driver_info ax88772b_info = {
.description = "ASIX AX88772B USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = ax88772_disable,
.status = asix_status,
.reset = ax88772_reset,
.stop = ax88772_stop,
@@ -1259,6 +1267,7 @@ static const struct driver_info hg20f9_info = {
.description = "HG20F9 USB 2.0 Ethernet",
.bind = ax88772_bind,
.unbind = ax88772_unbind,
+ .disable = 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 4ef61f6b85df..20ce88373809 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1223,7 +1223,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;

@@ -1997,7 +1997,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,
+ .disable = 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..5f3851e61573 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1641,17 +1641,21 @@ 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);

+ usb_kill_urb(dev->interrupt);
+
cancel_work_sync(&dev->kevent);

usb_scuttle_anchored_urbs(&dev->deferred);

- usb_kill_urb(dev->interrupt);
+ if (dev->driver_info->unbind)
+ dev->driver_info->unbind (dev, intf);
+
usb_free_urb(dev->interrupt);
kfree(dev->padding_pkt);

diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 1b4d72d5e891..dd4a1104e332 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