Messages in this thread | | | Date | Fri, 4 Mar 2011 15:10:31 -0800 | From | Greg KH <> | Subject | Re: [REVIEW] NVM Express driver |
| |
On Fri, Mar 04, 2011 at 10:33:29PM +0000, Alan Cox wrote: > > > How do most other apps do it. > > > > They write to the sysfs file with the firmware when they feel like it. > > Very few seem to do this. Quite a lot also have a much more complex > interaction anyway than 'firmware please', 'zap' , 'ok' to be fair. > > > > > So, what could be changed in the current firmware interface to fix this > > > > problem in a manner which would solve these perceived issues? > > > > > > I'm not sure it can. The basis of the interface is driver requests > > > firmware. That is done by using a pathname which in a namespaced > > > environment isn't global and has races > > > > Of course, but those races don't show up in real systems, right? > > They do. That's why I know they exist. If enough people use your distro > then eventually someone catches it.
Ok, I guess I must be doing support for distros that never get used :)
Have pointers to bug reports?
> > > (Several things break quite spectacularly if you request_firmware while > > > updating a package, but of course nobody considered such details even for > > > automatic stuff in many cases. Really there is some locking needed). > > > > I've never heard of this race before as you usually do firmware upload > > either at boot time, or when a user specifically asks for it. Neither > > of those times is when packages are usually getting updated. > > Usually is not a good answer for correctness and security with firmware. > It's really really not a good answer when flashing new firmware. "Usually > your expensive hardware is not turned into a valueless brick" lacks a > certain something. > > For dynamically requested firmware the request_firmware interface is > excellent stuff, and the right way to fix the races is to lock between > the daemon and package manager. For flashing stuff it's not up to the job.
Fair enough.
> > > For manual updating of a block of firmware the interface most stuff wants > > > is > > > > > > copy_from_user() > > > > > > or if you wanted to wrap it up nicely > > > > > > x = vmalloc_from_user(void __user *ptr, ssize_t len); > > > > > > The app knows which firmware it is talking about and can inspect and > > > compare it while holding an open file handle on the deivce. That protects > > > against hotplug races and getting the wrong device second access, and > > > ensures that the firmware, device and userspace are all talking about > > > exactly the same thing. > > > > But you do get this type of buffer from the firware interface to your > > driver today, right? > > Providing you jump through a small army of hoops, get the right sysfs > nodes, dodge any race conditions, hotplug and the like yes. But it should > be robust and simple. Using request_firmware for this case is neither > robust nor simple, nor is it easy to get the sysfs/driver open/hotplug > race handling right so instead of the kernel code being very occasionally > buggy (which we can spot) the user apps will be horribly buggy and we > don't read those so your push for a complex mis-standard in the kernel > actually makes the problem far worse than it would be without. > > > Still, I don't want this to all of a sudden be "open season" for every > > individual driver deciding to want to create an ioctl call for firmware > > updating just because the authors don't like the existing in-kernel > > interface. Please fix up the in-kernel one instead to meet your needs. > > You are not creating an open season. The every driver having its own > ioctl for firmware updating is the norm, and its in some ways a rather > good norm because when it comes to flash updating there isn't much > standardisation and you don't want standardisation as it just asks for > the wrong tool to work in an unfortunate race or user mix-up. Flash > updating is special - it's good that one app doesn't work on another > device ! > > You could have a standardised helper easily enough I guess. Pick a > standard struct and firmware descriptor and provide a > > request_firmware_from_user(struct firmware_something __user *fw) > > which hands back stuff in the form the rest of the firmware logic likes > to play and has a standard user side struct format.
Ok, patches gladly accepted :)
As well as the general "The firmware subsystem needs an active maintainer" plea, as the previous maintainer passed away a number of years ago :(
thanks,
greg k-h
| |