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: <87hatvdddl.fsf@nemi.mork.no>
Date:	Thu, 28 Jun 2012 10:55:34 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	Ming Lei <tom.leiming@...il.com>, 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

Oliver Neukum <oneukum@...e.de> writes:

> Am Dienstag, 26. Juni 2012, 09:23:19 schrieb Ming Lei:
>> On Mon, Jun 25, 2012 at 8:10 PM, Oliver Neukum <oliver@...kum.org> wrote:
>
>> > At this point a minidriver must not follow the intfdata pointer,
>> > because the interface may again be probed. So if here a minidriver
>> 
>> IMO, probe is serialized strictly with driver unbind since both the parent
>> lock and its own device lock have been held, so the probe may only be
>> started after driver unbinding is completed.
>
> Yes, but if you have a driver which claims multiple interfaces and uses
> a subdriver, then you will have cases of intfdate being NULL before
> disconnect() finishes.

This is true regardless of subdriver, isn't it?  The subdriver issue is
special because it makes this a potentional problem even with a single
interface.

>> > still uses intfdata, locking will be needed. We want to catch those
>> > casees.
>> 
>> Suppose infdata is used here somewhere, it is surely a bug because
>> the usbnet instance pointed by intfdata will be freed soon.
>
> Of course. That is the point.
>
>> So looks putting the set to NULL after driver_info->unbind is good,
>> doesn't it?
>
> Again, of course. We could drop it (but not the check for NULL in usbnet).
> It is a debugging aid. 
>
>> >        usb_kill_urb(dev->interrupt);
>> >        usb_free_urb(dev->interrupt);
>> >
>> >        free_netdev(net);
>> >        usb_put_dev (xdev);
>> > }
>> >
>> >> > Sure, it is a debugging aid. It has the drawback that minidrivers have
>> >> > to be able to deal with intfdata being NULL. That is not hard to do.
>> >>
>> >> The check isn't needed if the set to NULL is put after  driver_info->unbind
>> >> in usbnet_disconnect.
>> >
>> > True, but we don't catch bugs.
>> 
>> If the check is added, the bugs may be hided, and no stack will be
>> dumped, :-)
>
> That is also true.
>
> Bjørn,
>
> do you use subdrivers with cdc-ether?

No, and I don't think it should ever be added there.  It would add
complexity to the driver, and IMHO we really don't want that for the
generic cdc-ether. Better adding a new minidriver if the need to export
some other CDC embedded protocol to userspace shows up.

But given that all other embedded protocol examples I've seen are so
simple that they've been implemented inside their repective minidrivers
(sierra_net, hso, rndis_host), I don't think it's likely that any new
need for the cdc-wdm subdriver will show up soon.  I could of course be
completely wrong, as usual.


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