On Mon, 2009-11-30 at 10:12 -0500, Alan Stern wrote: > On Mon, 30 Nov 2009, David Vrabel wrote: > > > Alan Stern wrote: > > > Inaky and David: > > > > > > Looking through usb_authorize_device() and usb_deauthorize_device(), > > > those two routines appear to have several problems: That'd probably be my original fault, although it could also be things have changed since the original code was done. > >... > > * usb_authorize_device() renamed to usb_device_authenticated() and > > called from the WUSB core when a device is authenticated. > > There was a similar confusion at the time all this stuff was originally > merged. Inaky might have a different take on this, but as I recall he > explained that authentication and authorization are two separate and > more-or-less independent things. Authentication is the process of > determining that a USB device really is what it claims to be, and > authorization is the process of giving the system permission to use the > device. > > If you want to allow the system to use any device that has been > authenticated, then the authorization stuff isn't needed. Inaky had in > mind that system managers might want to prohibit use of certain USB > devices for administrative reasons, regardless of authentication. Yes, that was the original idea -- we also wanted to use the same mechanism to allow wired devices to be authorized to connect or not (to lock down systems). So in the case of WUSB, it was first authenticate it and once it is auth'ed, it is authorized to connect. > > It would also be nice to get the core to call usb_device_authenticated() > > for newly connected wired devices. The should reduce some code duplication. > > > > > > > > usb_authorize_device() calls usb_autoresume_device() but > > > not usb_autosuspend_device(). It this a simple oversight? > > > > I guess. I don't understand how this autosuspend thing is supposed to > > work though. > > ... I am not so sure either -- Sorry, I have not dived into the USB core code for the longest time and I forgot plenty of the details. > > > usb_deauthorize_device() frees a lot of refcounted data > > > structures without regard to the refcounts. Why doesn't > > > it leave them to be deallocated in the normal way? Sorry, blurry on the details after so long; this seems pretty basic stuff (USB core wise). I am surprised we didn't flag it during the patch reviews. When were all these structs made refcounted? I'd assume they have been like that for the longest time, so I wonder how could I have missed that :-/ > > > usb_deauthorize_device() leaks several strings. Oddly > > > enough, usb_authorize_device() handles them correctly. But > > > it races with the strings' show routines in sysfs.c, > > > because those routines don't acquire the device lock. > > > Shouldn't the show routines take the device lock then? > > That's the way to fix the problem. At the time those routines were > written, nothing else modified the strings. Hence there was no need > for them to take the device lock. Grr, bit rot > > > Does either of you want to fix these issues? > > > > I don't have time until Jan/Feb next year at the earliest. > > I can make the changes. I just wanted to check first and make sure > that there wouldn't be any objection. I am totally swamped with other stuff, so I won't be able, but I have no objections to any of the changes you guys are discussing. Heck, fixes are fixes :) Thanks for working on this, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
