| 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
| ||
|
Date: Wed, 4 Jun 2014 10:34:22 +0800
From: "Yang,Wei" <Wei.Yang@...driver.com>
To: "Yang,Wei" <Wei.Yang@...driver.com>,
Alan Stern <stern@...land.harvard.edu>,
Michal Nazarewicz <mina86@...a86.com>,
Andrzej Pietrasiewicz <andrzej.p@...sung.com>
CC: Felipe Balbi <balbi@...com>, <gregkh@...uxfoundation.org>,
USB list <linux-usb@...r.kernel.org>,
Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
Guys,
It seems the previous description is out of order. I describe it again.
Sorry for it.
irq handler
|
|-> s3c_hsotg_disconnect()
|
|-> common->new_fsg = NULL
|-> common->state = FSG_STATE_CONFIG
|-> wakes up fsg_main_thread.
|->set USB device address.
fsg_main_thread
|
|-> handle_exception
|
|-> common->state = FSG_STATE_IDLE
|-> do_set_interface()
irq happens -------------->
irq handler needs to handle USB_REQ_SET_CONFIGURATION request
|
|-> set_config()
|
|-> common->new_fsg = new_fsg;
|-> common->state = FSG_STATE_CONFIG
|-> cdev->delayed_status++
|-> wakes up fsg_main_thread
fsg_main_thread
|
|-> if(common->new_fsg)
|-> usb_composite_setup_continue()
|-> cdev->delayed_status--
|-> fsg_main_thread still finds the common->state is equal
to FSG_STATE_IDLE
|-> so it invokes handle_exception again, subsequently the
usb_composite_setup_continue
|-> is executed again. It would fininally results in the
warning.
Thanks
Wei
On 06/04/2014 09:20 AM, Yang,Wei wrote:
> On 06/03/2014 10:48 PM, Alan Stern wrote:
>> On Tue, 3 Jun 2014 Wei.Yang@...driver.com wrote:
>>
>>> From: Yang Wei <Wei.Yang@...driver.com>
>>>
>>> While loading g_mass_storage module, the following warning is
>>> triggered.
>>> In fact, it is more easy to reproduce it with RT kernel.
>>>
>>> WARNING: at drivers/usb/gadget/composite.c:
>>> usb_composite_setup_continue: Unexpected call
>>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1
>>> g_mass_storage
>>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>]
>>> (dump_stack+0x20/0x24)
>>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>]
>>> (warn_slowpath_common+0x64/0x74)
>>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>]
>>> (warn_slowpath_fmt+0x40/0x48)
>>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>]
>>> (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc
>>> [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4
>>> [g_mass_storage])
>>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from
>>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from
>>> [<8004bc90>] (kthread+0x98/0x9c)
>>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>]
>>> (kernel_thread_exit+0x0/0x8)
>>>
>>> The root cause just likes the following scenario.
>>>
>>> irq thread
>>>
>>> composite_disconnect()
>>> |
>>> |->fsg_disable() fsg->common->new_fsg = NULL
>>> and then wake fsg_main_thread
>>> with seting common->state to
>>> FSG_STATE_CONFIG_CHANGE.
>>> fsg_main_thread
>>> |
>>> |->do_set_interface()
>>> irq thread
>>>
>>> set_config()
>>> |
>>> |->fsg_set_alt() fsg->common->new_fsg = new_fsg
>>> and then also wake up fsg_main_thread
>>> with setting common->state to
>>> FSG_STATE_CONFIG_CHANGE.
>>> |->
>>> if(common->new_fsg)
>>> usb_composite_setup_continue()
>>>
>>> In this case, fsg_main_thread would invoke usb_composite_setup_continue
>>> twice, so the second call would trigger the above call trace, as we
>>> also
>>> save common->new_fsg while changing the common->state.
>> Michal and Andrzej:
>>
>> I haven't paid much attention to these matters, because you handled the
>> conversion from g_file_storage to f_mass_storage using the composite
>> framework. But this patch seemed odd, so I took a closer look.
>
> Let me make sense it, Robert once introduced the following patch to
> fix disconnect handling of s3c-hsotg.
>
> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> Author: Robert Baldyga <r.baldyga@...sung.com>
> Date: Thu Nov 21 13:49:18 2013 +0100
>
> usb: gadget: s3c-hsotg: fix disconnect handling
>
> This patch moves s3c_hsotg_disconnect function call from USBSusp
> interrupt
> handler to SET_ADDRESS request handler.
>
> It's because disconnected state can't be detected directly,
> because this
> hardware doesn't support Disconnected interrupt for device mode.
> For both
> Suspend and Disconnect events there is one interrupt USBSusp, but
> calling
> s3c_hsotg_disconnect from this interrupt handler causes config
> reset in
> composite layer, which is not undesirable for Suspended state.
>
> For this reason s3c_hsotg_disconnect is called from SET_ADDRESS
> request
> handler, which occurs always after disconnection, so we do disconnect
> immediately before we are connected again. It's probably only way we
> can do handle disconnection correctly.
>
> Signed-off-by: Robert Baldyga <r.baldyga@...sung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@...sung.com>
> Signed-off-by: Felipe Balbi <balbi@...com>
>
> Just like what the commit log described, s3c_hsotg_disconnect is
> called from SET_ADDRESS request handler, therefore,
> reset_config would finally be called, it raises a
> FSG_STATE_CONFIG_CHANGE exception and wakes up fsg_main_thread to
> handle this exception. After handling SET_ADDRESS, subsequently the
> irq hanler of s3c-hsotg would also invokes composite_setup()
> function to handle USB_REQ_SET_CONFIGURATION request, set_config would
> be invoked, it also raises a FSG_STATE_CONFIG_CHANGE
> exception and wakes up fsg_main_thread, in mean time,
> cdev->delayed_status would be plus one. Right? If I am missing
> something, please
> let me know it.:-) If so, the following scenario would trigger the
> above call trace.
>
> irq handler
> |
> |-> s3c_hsotg_disconnect()
> |
> |-> common->new_fsg = NULL
> |-> common->state to FSG_STATE_CONFIG.
> |-> wakes up fsg_main_thread.
> |-> set USB device address
>
> fsg_main_thread finds the common->state == FSG_STATE_CONFIG
> |
> |-> handle_execption
> |
> |-> set common->state to FSG_STATE_IDLE
> irq hanppens ------------>|
> irq handler needs to hanle USB_REQ_SET_CONFIGURATION request.
> |->do_set_interface()
> |-> set_config()
> |
> |-> common->new_fsg = new_fsg;
> |-> common->state = FSG_STATE_CONFIG
> |-> cdev->delayed_status++
> |-> wakes up fsg_main_thread
>
> |-> Now the common->state == FSG_STATE_CONFIG
> |-> if(common->new_fsg)
> usb_composite_setup_continue()
> |->cdev->delayed_status--
> |
> fsg_main_thread finds the common->state still is FSG_STATE_CONFIG,
> | so it would invoke handle_execption again.
> |->hanle_execption
> |-> set common->state to FSG_STATE_IDLE
> |-> do_set_interface()
> |-> if (common->new_fsg)
> usb_composite_setup_continue()
> |-> cdev->delayed_status
> == 0, so
> this warning is triggered.
>
>
> Thanks
> Wei
>
>>
>> In f_mass_storage.c, struct fsg_common is shared among all the function
>> instances. This structure includes things like cmnd and nluns, which
>> will in general be different for different functions.
>>
>> That's okay if each function is in a separate config, but what happens
>> when there are multiple functions in the same config, using different
>> interfaces? What if the host sends concurrent commands to two of these
>> functions?
>>
>> Am I missing something?
>>
>> Alan Stern
>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists