procfs: Kill the bkl in ioctl
in [Kernel]
|
Prev: procfs: Kill BKL in llseek on proc base
Next: net/wireless/libertas: do not call wiphy_unregister() w/o wiphy_register()
From: Christoph Hellwig on 11 Apr 2010 09:10 On Sat, Apr 10, 2010 at 05:28:16PM +0200, Frederic Weisbecker wrote: > So you mean we should attribute explicit default_llseek to the evil > places instead of explicit generic_file_llseek in the safe ones? > That's not a bad idea as it would result in much less changes. > > The problem happens the day you switch to generic_file_llseek() as the > new default llseek(), how do you prove that all remaining fops > that don't implement .llseek don't use the bkl? There will be > hundreds of them and saying "we've looked all of them and they don't > need it" will be a scary justification. > > On the opposite, attributing explicit generic_file_llseek or > non_seekable_open on the safe places and default_llseek on > the dozens of others doubtful places is easier to get a > safe conclusion. > > But yeah we should try, at least attributing explicit > default_llseek won't harm, quite the opposite. Note that an lssek that actually does something is the wrong default, even if we have it that way currently. If the default is changed it should be changed to give the semantics that nonseekable_open() gives us. Given that you guys are so motivated to do something in this area it might be a good idea to do this in a few simple steps: - make sure every file operation either has a ->llseek instead or calls nonseekable_open from ->open - remove nonseekable_open and all calls to it - switch all users of no_llseek to not set a ->llsek after auditing that there's no corner case where we want to allow pread/pwrite but not lseek, which is rather unlikely - walk through the instances now using default_llseek and chose a better implementation for this particular instance. Often this will be just removing the the lssek method as not allowing seeks is the right thing to do for character drivers, even if it is a behaviour change from the current version which usually is the result of sloppy coding. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 12 Apr 2010 11:10 On Saturday 10 April 2010, Frederic Weisbecker wrote: > Do you mind if I rename this to deprecated_ioctl()? > This "default" naming suggests a fallback everyone that don't > need tricky things should use. Sounds good. The idea was to keep the naming consistent with default_llseek, but after the discussion on llseek, we will probably do something else with it anyway. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 12 Apr 2010 13:40 On Sunday 11 April 2010, Christoph Hellwig wrote: > On Sat, Apr 10, 2010 at 05:28:16PM +0200, Frederic Weisbecker wrote: > > So you mean we should attribute explicit default_llseek to the evil > > places instead of explicit generic_file_llseek in the safe ones? > > That's not a bad idea as it would result in much less changes. > > > > The problem happens the day you switch to generic_file_llseek() as the > > new default llseek(), how do you prove that all remaining fops > > that don't implement .llseek don't use the bkl? There will be > > hundreds of them and saying "we've looked all of them and they don't > > need it" will be a scary justification. > > > > On the opposite, attributing explicit generic_file_llseek or > > non_seekable_open on the safe places and default_llseek on > > the dozens of others doubtful places is easier to get a > > safe conclusion. > > > > But yeah we should try, at least attributing explicit > > default_llseek won't harm, quite the opposite. > > Note that an lssek that actually does something is the wrong default, > even if we have it that way currently. If the default is changed it > should be changed to give the semantics that nonseekable_open() > gives us. Given that you guys are so motivated to do something in > this area it might be a good idea to do this in a few simple steps: > > - make sure every file operation either has a ->llseek instead or > calls nonseekable_open from ->open I still think it would be better to always set llseek if we do that, even if nonseekable_open is already there. I can come up with scripts that check that case, but checking that the open function always calls nonseekable_open when it returns success is beyond my grep skills ;-) > - remove nonseekable_open and all calls to it > - switch all users of no_llseek to not set a ->llsek after auditing > that there's no corner case where we want to allow pread/pwrite > but not lseek, which is rather unlikely This parts seems fine. > - walk through the instances now using default_llseek and chose > a better implementation for this particular instance. Often > this will be just removing the the lssek method as not allowing > seeks is the right thing to do for character drivers, even if it > is a behaviour change from the current version which usually > is the result of sloppy coding. This part is really hard. While in many cases, the driver maintainer might know what user space is potentially opening some character device, it's really hard to tell for outsiders whether the behaviour should be no_llseek (then the default) or noop_llseek to work around broken user space. I think the rule set for the conversion needs to be one that can be done purely based on the code. How about this: For each file operation { if (uses f_pos) { if (same module uses BKL) -> default_llseek else -> generic_file_llseek } else { if (driver maintained) -> no_llseek (with maintainer ACK) else -> noop_llseek } } Once that is done, we can turn the default into nonseekable behavior and start removing instances of explicit no_llseek and nonseekable_open. Should we also rename default_llseek to deprecated_llseek in the process, to go along with the approach for ioctl? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 12 Apr 2010 18:00 On Mon, Apr 12, 2010 at 07:34:17PM +0200, Arnd Bergmann wrote: > On Sunday 11 April 2010, Christoph Hellwig wrote: > > On Sat, Apr 10, 2010 at 05:28:16PM +0200, Frederic Weisbecker wrote: > > > So you mean we should attribute explicit default_llseek to the evil > > > places instead of explicit generic_file_llseek in the safe ones? > > > That's not a bad idea as it would result in much less changes. > > > > > > The problem happens the day you switch to generic_file_llseek() as the > > > new default llseek(), how do you prove that all remaining fops > > > that don't implement .llseek don't use the bkl? There will be > > > hundreds of them and saying "we've looked all of them and they don't > > > need it" will be a scary justification. > > > > > > On the opposite, attributing explicit generic_file_llseek or > > > non_seekable_open on the safe places and default_llseek on > > > the dozens of others doubtful places is easier to get a > > > safe conclusion. > > > > > > But yeah we should try, at least attributing explicit > > > default_llseek won't harm, quite the opposite. > > > > Note that an lssek that actually does something is the wrong default, > > even if we have it that way currently. If the default is changed it > > should be changed to give the semantics that nonseekable_open() > > gives us. Given that you guys are so motivated to do something in > > this area it might be a good idea to do this in a few simple steps: > > > > - make sure every file operation either has a ->llseek instead or > > calls nonseekable_open from ->open > > I still think it would be better to always set llseek if we do that, > even if nonseekable_open is already there. I can come up with scripts > that check that case, but checking that the open function always > calls nonseekable_open when it returns success is beyond my grep > skills ;-) > > > - remove nonseekable_open and all calls to it > > - switch all users of no_llseek to not set a ->llsek after auditing > > that there's no corner case where we want to allow pread/pwrite > > but not lseek, which is rather unlikely > > This parts seems fine. > > > - walk through the instances now using default_llseek and chose > > a better implementation for this particular instance. Often > > this will be just removing the the lssek method as not allowing > > seeks is the right thing to do for character drivers, even if it > > is a behaviour change from the current version which usually > > is the result of sloppy coding. > > This part is really hard. While in many cases, the driver maintainer > might know what user space is potentially opening some character > device, it's really hard to tell for outsiders whether the behaviour > should be no_llseek (then the default) or noop_llseek to work around > broken user space. Also even if llseek is useless for a module, turning it into unseekable somehow changes the userspace ABI. I guess this is harmless 99% of the time, but still. And maintainers tend not to like that. > > I think the rule set for the conversion needs to be one that can > be done purely based on the code. How about this: > > For each file operation { > if (uses f_pos) { > if (same module uses BKL) > -> default_llseek > else > -> generic_file_llseek > } else { > if (driver maintained) > -> no_llseek (with maintainer ACK) > else > -> noop_llseek > } > } It is also hard to determine a given driver really doesn't use the bkl. A sole lock_kernel() grep in its files is not sufficient. But a manual second pass should do the trick. > > Once that is done, we can turn the default into nonseekable > behavior and start removing instances of explicit no_llseek > and nonseekable_open. Sounds good. > Should we also rename default_llseek to deprecated_llseek in the > process, to go along with the approach for ioctl? Yeah, preferably. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 13 Apr 2010 05:30
On Monday 12 April 2010, Frederic Weisbecker wrote: > On Mon, Apr 12, 2010 at 07:34:17PM +0200, Arnd Bergmann wrote: > > > > I think the rule set for the conversion needs to be one that can > > be done purely based on the code. How about this: > > > > For each file operation { > > if (uses f_pos) { > > if (same module uses BKL) > > -> default_llseek > > else > > -> generic_file_llseek > > } else { > > if (driver maintained) > > -> no_llseek (with maintainer ACK) > > else > > -> noop_llseek > > } > > } > > It is also hard to determine a given driver really doesn't use > the bkl. A sole lock_kernel() grep in its files is not sufficient. > But a manual second pass should do the trick. Why not? In my 2.6.33 based series, I have removed all implicit uses of the BKL, so we can be sure that it doesn't use the BKL unless the module is part of that series. The only two cases I can think of are: - ioctl callback, which we should do in the same change, like I originally did. If a driver defines ->ioctl(), make it use deprecated_ioctl() and default_llseek()/deprecated_llseek. - Any of the file systems from Jan's series. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |