Re: [PATCH v2] usb: mtu3: fix panic in mtu3_gadget_disconnect()

From: Alan Stern <stern@rowland.harvard.edu>
To: Macpaul Lin <macpaul.lin@mediatek.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>,
	Eddie Hung <eddie.hung@mediatek.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	Mediatek WSD Upstream <wsd_upstream@mediatek.com>,
	Macpaul Lin <macpaul.lin@gmail.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] usb: mtu3: fix panic in mtu3_gadget_disconnect()
Date: Fri, 31 Jul 2020 10:22:12 -0400	[thread overview]
Message-ID: <20200731142212.GE36650@rowland.harvard.edu> (raw)
In-Reply-To: <1596185878-24360-1-git-send-email-macpaul.lin@mediatek.com>

On Fri, Jul 31, 2020 at 04:57:58PM +0800, Macpaul Lin wrote:
> This patch fixes a possible issue when mtu3_gadget_stop()
> already assigned NULL to mtu->gadget_driver during mtu_gadget_disconnect().

> 
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> Cc: stable@vger.kernel.org
> ---
> Changes for v2:
>   - Check mtu_gadget_driver out of spin_lock might still not work.
>     We use a temporary pointer to keep the callback function.
> 
>  drivers/usb/mtu3/mtu3_gadget.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_gadget.c b/drivers/usb/mtu3/mtu3_gadget.c
> index 68ea4395f871..40cb6626f496 100644
> --- a/drivers/usb/mtu3/mtu3_gadget.c
> +++ b/drivers/usb/mtu3/mtu3_gadget.c
> @@ -840,10 +840,17 @@ void mtu3_gadget_suspend(struct mtu3 *mtu)
>  /* called when VBUS drops below session threshold, and in other cases */
>  void mtu3_gadget_disconnect(struct mtu3 *mtu)
>  {
> +	struct usb_gadget_driver *driver;
> +
>  	dev_dbg(mtu->dev, "gadget DISCONNECT\n");
>  	if (mtu->gadget_driver && mtu->gadget_driver->disconnect) {
> +		driver = mtu->gadget_driver;
>  		spin_unlock(&mtu->lock);
> -		mtu->gadget_driver->disconnect(&mtu->g);
> +		/*
> +		 * avoid kernel panic because mtu3_gadget_stop() assigned NULL
> +		 * to mtu->gadget_driver.
> +		 */
> +		driver->disconnect(&mtu->g);
>  		spin_lock(&mtu->lock);
>  	}

This is not the right approach; it might race with the gadget driver 
unregistering itself.

Instead, mtu3_gadget_stop() should call synchronize_irq() after 
releasing the IRQ line.  When synchronize_irq() returns, you'll know any 
IRQ handlers have finished running, so you won't receive any more 
disconnect notifications.  Then it will be safe to acquire the spinlock 
and set mtu->gadget_driver to NULL.

Alan Stern