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: <87txy3jlrw.fsf@nemi.mork.no>
Date:	Fri, 22 Jun 2012 19:31:47 +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 Sat, Jun 23, 2012 at 12:18 AM, Bjørn Mork <bjorn@...k.no> wrote:
>> Ming Lei <tom.leiming@...il.com> writes:
>
>> Yes, intfdata is per interface, but in the case of cdc_ether (and
>> probably other similar minidrivers) there are two interfaces pointing to
>> the *same* usbnet private data.
>
> Yes, the priv pointers of both interface points to the usbnet instance, but
> the same pointer is stored into two places.
>
>>
>> What about the situation where disconnect is called simultaneously for
>> both interfaces? Or can't that happen? If it can, then we'll do
>
> It can't happen because both parent lock and its lock need to be held in
> hotplug situation or unbind situation.

OK, then it can probably go away.  But then again it doesn't do any harm
for any other minidriver.  qmi_wwan is special because of the
cooperation with cdc_wdm.

>>  driver->disconnect(intf1)                driver->disconnect(intf2)
>>     dev = usb_get_intfdata(intf1)           dev = usb_get_intfdata(intf2)
>>     dev->driver_info->unbind()              dev->driver_info->unbind()
>>     net = dev->net                          net = dev->net
>>     free_netdev(net)                        free_netdev(dev->net)
>>
>> where "dev" and "net" will be pointing to the same private data and
>> netdevice.
>
> Suppose driver->disconnect(intf1) is called first, .ubind() inside
> .disconnect will clear intfdata of another interface(intf2) and call
> usb_driver_release_interface(intf2), which will cause .disconnect(intf2)
> called, but it will return immediately.

Yes, if that's the only possible call sequence then I agree that
removing the call won't be a problem.  But I don't see much gain either.
And it has been there since the beginning of git history, if that counts
for anything around here.

>> I assume that is what the code above is trying to protect against.  But
>
> I don't see the protection in the code, :-)

I am trying my best to be positive :-)


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