linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Oliver Neukum <oneukum@suse.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Oleksij Rempel <linux@rempel-privat.de>,
	Dongliang Mu <dzm91@hust.edu.cn>,
	Oliver Neukum <oliver@neukum.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Dongliang Mu <mudongliangabcd@gmail.com>,
	syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com,
	USB <linux-usb@vger.kernel.org>, netdev <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
Date: Wed, 20 Apr 2022 08:56:52 +0200	[thread overview]
Message-ID: <Yl+utFmKEgILDFr5@hovoldconsulting.com> (raw)
In-Reply-To: <d851497f-7960-b606-2f87-eb9bff89c8ac@suse.com>

On Tue, Apr 19, 2022 at 01:47:40PM +0200, Oliver Neukum wrote:
> 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.

Yes, indeed, commit 2c9d6c2b871d ("usbnet: run unbind() before
unregister_netdev()") is fundamentally broken. You can't just start
freeing driver private data before deregistering 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?

That sounds correct. I only noticed that the proposed patch looked
insufficient at best and didn't really look into the backstory until
just now.

> 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.

I agree, we need to strive to maintain symmetry. Anything else is likely
broken and at least makes things harder to reason about and maintenance
a pain.

> 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

> 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,

These should all be

	.disable = ...

but you probably need to split the callback and keep unbind as well for
the actual clean up (freeing resources etc).

>  	.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);

Don't you need to quiesce all I/O, including stopping the interrupt URB,
before unbind?

>  	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 *);

Johan

  parent reply	other threads:[~2022-04-20  6:57 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-09 12:09 [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free Dongliang Mu
2022-04-11 12:14 ` Johan Hovold
2022-04-14 13:58   ` Dongliang Mu
2022-04-14 14:03     ` Dongliang Mu
2022-04-14 15:01   ` Andy Shevchenko
2022-04-15  7:19     ` Oleksij Rempel
2022-04-19 11:47     ` Oliver Neukum
2022-04-19 20:25       ` Bjørn Mork
2022-04-20  6:56       ` Johan Hovold [this message]
2022-04-20  9:45         ` Oliver Neukum
2022-04-20 10:06           ` Johan Hovold
2022-04-21 11:18     ` Oliver Neukum
2022-04-11 14:51 ` Andy Shevchenko
2022-04-14 13:59   ` Dongliang Mu

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=Yl+utFmKEgILDFr5@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=davem@davemloft.net \
    --cc=dzm91@hust.edu.cn \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@rempel-privat.de \
    --cc=mudongliangabcd@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=oliver@neukum.org \
    --cc=oneukum@suse.com \
    --cc=pabeni@redhat.com \
    --cc=syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.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).