diff mbox

[4.4,18/31] r8152: napi hangup fix after disconnect

Message ID 20180720121340.812996969@linuxfoundation.org (mailing list archive)
State New, archived
Headers show

Commit Message

Greg Kroah-Hartman July 20, 2018, 12:13 p.m. UTC 
4.4-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Jiri Slaby <jslaby@suse.cz>

[ Upstream commit 0ee1f4734967af8321ecebaf9c74221ace34f2d5 ]

When unplugging an r8152 adapter while the interface is UP, the NIC
becomes unusable.  usb->disconnect (aka rtl8152_disconnect) deletes
napi. Then, rtl8152_disconnect calls unregister_netdev and that invokes
netdev->ndo_stop (aka rtl8152_close). rtl8152_close tries to
napi_disable, but the napi is already deleted by disconnect above. So
the first while loop in napi_disable never finishes. This results in
complete deadlock of the network layer as there is rtnl_mutex held by
unregister_netdev.

So avoid the call to napi_disable in rtl8152_close when the device is
already gone.

The other calls to usb_kill_urb, cancel_delayed_work_sync,
netif_stop_queue etc. seem to be fine. The urb and netdev is not
destroyed yet.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/usb/r8152.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ben Hutchings Aug. 24, 2018, 4:38 p.m. UTC | #1 
On Fri, 2018-07-20 at 14:13 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Jiri Slaby <jslaby@suse.cz>
> 
> [ Upstream commit 0ee1f4734967af8321ecebaf9c74221ace34f2d5 ]
[...]
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -3139,7 +3139,8 @@ static int rtl8152_close(struct net_devi
>  #ifdef CONFIG_PM_SLEEP
>  	unregister_pm_notifier(&tp->pm_notifier);
>  #endif
> -	napi_disable(&tp->napi);
> +	if (!test_bit(RTL8152_UNPLUG, &tp->flags))
> +		napi_disable(&tp->napi);
>  	clear_bit(WORK_ENABLE, &tp->flags);
>  	usb_kill_urb(tp->intr_urb);
>  	cancel_delayed_work_sync(&tp->schedule);

This flag appears to be set only if the USB device is actually
disconnected.  In case the driver is unbound for some other reason
(like the module is removed), the same problem will occur.

What I think might work is to do:

	if (!list_empty(&tp->napi.dev_list)
		napi_disable(&tp->napi);

Ben.
Jiri Slaby Aug. 25, 2018, 7:43 a.m. UTC | #2 
On 08/24/2018, 06:38 PM, Ben Hutchings wrote:
> On Fri, 2018-07-20 at 14:13 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Jiri Slaby <jslaby@suse.cz>
>>
>> [ Upstream commit 0ee1f4734967af8321ecebaf9c74221ace34f2d5 ]
> [...]
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -3139,7 +3139,8 @@ static int rtl8152_close(struct net_devi
>>  #ifdef CONFIG_PM_SLEEP
>>  	unregister_pm_notifier(&tp->pm_notifier);
>>  #endif
>> -	napi_disable(&tp->napi);
>> +	if (!test_bit(RTL8152_UNPLUG, &tp->flags))
>> +		napi_disable(&tp->napi);
>>  	clear_bit(WORK_ENABLE, &tp->flags);
>>  	usb_kill_urb(tp->intr_urb);
>>  	cancel_delayed_work_sync(&tp->schedule);
> 
> This flag appears to be set only if the USB device is actually
> disconnected.  In case the driver is unbound for some other reason
> (like the module is removed), the same problem will occur.

Could you elaborate? I thought this would happen:
module_exit -> usb_deregister -> usb_unbind_device -> rtl8152_disconnect
-> unregister_netdev -> rtl8152_close

Am I missing something?

thanks,
Ben Hutchings Sept. 12, 2018, 6:54 p.m. UTC | #3 
On Sat, 2018-08-25 at 09:43 +0200, Jiri Slaby wrote:
> On 08/24/2018, 06:38 PM, Ben Hutchings wrote:
> > On Fri, 2018-07-20 at 14:13 +0200, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Jiri Slaby <jslaby@suse.cz>
> > > 
> > > [ Upstream commit 0ee1f4734967af8321ecebaf9c74221ace34f2d5 ]
> > 
> > [...]
> > > --- a/drivers/net/usb/r8152.c
> > > +++ b/drivers/net/usb/r8152.c
> > > @@ -3139,7 +3139,8 @@ static int rtl8152_close(struct net_devi
> > >  #ifdef CONFIG_PM_SLEEP
> > >  	unregister_pm_notifier(&tp->pm_notifier);
> > >  #endif
> > > -	napi_disable(&tp->napi);
> > > +	if (!test_bit(RTL8152_UNPLUG, &tp->flags))
> > > +		napi_disable(&tp->napi);
> > >  	clear_bit(WORK_ENABLE, &tp->flags);
> > >  	usb_kill_urb(tp->intr_urb);
> > >  	cancel_delayed_work_sync(&tp->schedule);
> > 
> > This flag appears to be set only if the USB device is actually
> > disconnected.  In case the driver is unbound for some other reason
> > (like the module is removed), the same problem will occur.
> 
> Could you elaborate? I thought this would happen:
> module_exit -> usb_deregister -> usb_unbind_device -> rtl8152_disconnect
> -> unregister_netdev -> rtl8152_close
> 
> Am I missing something?

What I mean is that if the USB device has not been *physically*
disconnected then its usb_device::state will not be
USB_STATE_NOTATTACHED.  So rtl8152_disconnect() will not set the
RTL8152_UNPLUG flag and rtl8152_close() will still call napi_disable()
which will hang.

Some options to fix this:
- Add a separate flag which rtl8152_close() checks and
rtl8152_disconnect() always sets
- Call dev_close() before netif_napi_del()

Ben.
diff mbox

Patch

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3139,7 +3139,8 @@  static int rtl8152_close(struct net_devi
 #ifdef CONFIG_PM_SLEEP
 	unregister_pm_notifier(&tp->pm_notifier);
 #endif
-	napi_disable(&tp->napi);
+	if (!test_bit(RTL8152_UNPLUG, &tp->flags))
+		napi_disable(&tp->napi);
 	clear_bit(WORK_ENABLE, &tp->flags);
 	usb_kill_urb(tp->intr_urb);
 	cancel_delayed_work_sync(&tp->schedule);