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]

Date:	Fri, 22 Jun 2012 23:09:36 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Bjørn Mork <bjorn@...k.no>
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

On Fri, Jun 22, 2012 at 9:42 PM, Bjørn Mork <bjorn@...k.no> wrote:
> 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

Suppose intfdata is not used in .open/.close, there are no the race.

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

Yes.

Looks I understand the problem incorrectly, your problem is
that qmi_wwan_cdc_wdm_manage_power is called from
wdm_open, not from usbnet_open. It is a problem crossing
2 class drivers.

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

No, intfdata is per interface, and .unbind will clear it for each interface.

> So I don't think it can be removed.  Not without touching the

It can be removed, but not necessary since it can't fix your problem.


Thanks,
-- 
Ming Lei
--
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