netdev - Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mx3vlayl.fsf@nemi.mork.no>
Date:	Fri, 22 Jun 2012 15:42:26 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Marius Bjørnstad Kotsbak 
	<marius.kotsbak@...il.com>
Subject: Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

Ming Lei <tom.leiming@...il.com> writes:

> On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@...k.no> wrote:
>> usbnet_disconnect() will set intfdata to NULL before calling
>> the minidriver unbind function.  The cdc_wdm subdriver cannot
>> know that it is disconnecting until the qmi_wwan unbind
>> function has called its disconnect function.  This means that
>> we must be able to support the cdc_wdm subdriver operating
>> normally while usbnet_disconnect() is running, and in
>> particular that intfdata may be NULL.
>>
>> The only place this matters is in qmi_wwan_cdc_wdm_manage_power
>> which is called from cdc_wdm.  Simply testing for NULL
>> intfdata there is sufficient to allow it to continue working
>> at all times.
>>
>> Fixes this Oops where a cdc-wdm device was closed while the
>> USB device was disconnecting, causing wdm_release to call
>> qmi_wwan_cdc_wdm_manage_power after intfdata was set to
>> NULL by usbnet_disconnect:
>
> In fact, it should be a general problem in usbnet, there are races
> between usbnet_disconnect and .open/.close. Considered that
> unregister_netdev, which will sync with .open/.close,  is called in
> usbnet_disconnect,  the simplest fix is to move usb_set_intfdata(NULL)
> after unregister_netdev.

Is there really a race there?  The usbnet .open/.close don't use the
intfdata, do they?  I looked briefly through usbnet for related
potentional problems while fixing this in qmi_wwan, but could only find
suspend/resume.  Which I believe are protected against running on
disconnect.

So I think usbnet in general is OK.

> Considered that usb_set_intfdata(NULL) will be called after
> executing .disconnect in unbind path, it can be removed simply.
>
> So how about the below?
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index e92c057..2eb9e1e 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf)
>  	struct net_device	*net;
>
>  	dev = usb_get_intfdata(intf);
> -	usb_set_intfdata(intf, NULL);
>  	if (!dev)
>  		return;

I believe that call is there to prevent disconnect from running twice
for the common two-interface CDC ethernet model, like e.g. cdc_ether.
So I don't think it can be removed.  Not without touching the
minidrivers depending on that behaviour, anyway.


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