linux-kernel - Re: [PATCH] usb: f_mass_storage: test whether thread is running before starting another

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]

Date:	Tue, 5 Apr 2016 14:35:19 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Michal Nazarewicz <mina86@...a86.com>
cc:	Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>,
	Tony Lindgren <tony@...mide.com>,
	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	Felipe Balbi <felipe.balbi@...ux.intel.com>,
	Bin Liu <b-liu@...com>, <pali.rohar@...il.com>
Subject: Re: [PATCH] usb: f_mass_storage: test whether thread is running
 before starting another

On Tue, 5 Apr 2016, Michal Nazarewicz wrote:

> > On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
> >> When binding the function to usb_configuration, check whether the thread
> >> is running before starting another one.  Without that, when function
> >> instance is added to multiple configurations, fsg_bing starts multiple
> >> threads with all but the latest one being forgotten by the driver.  This
> >> leads to obvious thread leaks, possible lockups when trying to halt the
> >> machine and possible more issues.
> >> 
> >> This fixes issues with legacy/multi¹ gadget as well as configfs gadgets
> >> when mass_storage function is added to multiple configurations.
> >> 
> >> This change also simplifies API since the legacy gadgets no longer need
> >> to worry about starting the thread by themselves (which was where bug
> >> in legacy/multi was in the first place).
> >> 
> >> ¹ I have no example failure though.  Conclusion that legacy/multi has
> >>   a bug is based purely on me reading the code.
> >> 
> >> Signed-off-by: Michal Nazarewicz <mina86@...a86.com>
> 
> On Tue, Apr 05 2016, Alan Stern wrote:
> > This doesn't address the problem I raised in a previous email.  
> > Sharing one thread among several function instances in the same config
> > will not work if one of them encounters an error.
> 
> Each usb_function_instance has its own fsg_common and its own thread.
> This was true in the past and is true with this patch as well.

We're getting confused by the stupid terminology again.  Yes, each
usb_function_instance has its own fsg_common and its own thread.  But
multiple usb_function structures (each one being a separate function
instance) can belong to the same usb_function_instance and they will
all share the same fsg_common.

That's what happened with the nokia driver.  It creates one 
usb_function_instance with two usb_function structures.  In this case 
they are in different configs, so sharing a thread doesn't matter.  But 
it would matter if they were in the same config.

> And unless I’m missing something, sharing a thread among multiple
> usb_function’s does not prevent the driver from working correctly.

Suppose one usb_function is carrying out an I/O operation while another
one in the same config gets a Set-Interface request from the host.  
The request causes the driver to raise an FSG_STATE_CONFIG_CHANGE
exception.  When the thread sees this exception, it will abort the I/O
that it is carrying out for the first usb_function.

In other words, exceptions raised by one instance will affect the 
shared thread even when it's doing work for a different instance.

> Having the thread run even when it’s not used may be considered wasteful
> but that’s an orthogonal issue to the configfs failure.

Yes.  Having extra unused threads isn't terrible.

Alan Stern