netdev - Re: Active URB submitted twice in pegasus driver

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: <alpine.DEB.2.02.1303261909260.4226@fry.nucleusys.com>
Date:	Tue, 26 Mar 2013 19:28:53 +0200 (EET)
From:	Petko Manolov <petkan@...leusys.com>
To:	Sarah Sharp <sarah.a.sharp@...ux.intel.com>
cc:	linux-usb@...r.kernel.org, Greg KH <gregkh@...uxfoundation.org>,
	netdev@...r.kernel.org,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: Active URB submitted twice in pegasus driver

On Tue, 26 Mar 2013, Sarah Sharp wrote:

> On Tue, Mar 26, 2013 at 05:22:07PM +0200, Petko Manolov wrote:
>> On Mon, 25 Mar 2013, Sarah Sharp wrote:
>>
>>> Hi Petko,
>>>
>>> I'm testing a USB to ethernet adapter with Greg's usb-linus branch (based
>>> on 3.9-rc4).  I'm seeing an odd behavior, and I'm suspicious that a
>>> second behavior found by Stephen Hemminger may also be related:
>>>
>>> http://marc.info/?l=linux-usb&m=136364625519235&w=2
>>>
>>> When I load the pegasus driver for this adapter (without an ethernet
>>> cable hooked to it), and this is on a cold system boot or a complete
>>> unloading and reloading of the usbcore, I get the following warning:
>>
>> Unfortunately the machine i ran my tests on does not have xHCI nor i
>> used Greg's tree.  I did not observe this warning on both 3.8.4 and
>> Linus' 3.9-rc4.  It looks like to me that the issue is outside the
>> driver code.
>
> Or it could be a race condition that's only triggered by the xHCI driver
> (or me having xHCI debugging turned on).  Control transfer completions
> may be delayed when I have debug turned on.

They most likely are.  However, considering the stuff below these delays 
may not be the real cause.

> The only other USB to ethernet adapter I have is also an ADMteck device:
>
> Bus 001 Device 008: ID 07a6:8513 ADMtek, Inc. AN8513 Ethernet

I've got another one, rtl8150 based adapter.  The way things look i'll 
have to do some testing on my own.

> What do you mean by an asynchronous call?  Do you mean that
> xxx_set_multicast() could be called at any time?

Yep.  The first call to pegasus_set_multicast() is done by the network 
layer upon device insertion and those often happen outside process 
context.

> After taking a brief glance at the pegasus code, pegasus_set_multicast 
> looks broken.  It sets the control URB status to zero and calls 
> ctrl_callback(), which does some stuff if the URB status is zero.  It 
> doesn't look like pegasus_set_multicast() is an URB callback function, 
> so why in the world is it touching a control URB that could possibly be 
> in flight else where?

Beats me, i've got nothing to do with this driver. :-P

Anyway, playing with the URB status looks rather stupid and is definitely 
a bug.  I'll make a patch that fixes the problem.

> It looks like the control URB is used to do things like get or set 
> registers, so what's stopping the upper layers from calling get 
> registers (which will submit the control URB and schedule a wait queue), 
> and then pegasus_set_multicast(), which will go overwrite the active URB 
> status?  USB device drivers should not be writing or reading fields in a 
> submitted URB until the completion handler's callback function is 
> called.

Err, see my previous comment.  It can't be me.  Maybe i've been too drunk. 
Or stupid.  Or both... :-)
--
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