| 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 09:20:27 +0800
From: "Yang,Wei" <Wei.Yang@...driver.com>
To: 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
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