Frederic Weisbecker wrote:
> On Tue, Mar 30, 2010 at 11:33:40AM +0100, Arnd Bergmann wrote:
>> I believe we can actually remove ioctl from file_operations. The patch I did
>> to convert all users to ".unlocked_ioctl = default_ioctl," should really catch
>> all cases, and I think we can enforce this by renaming fops->ioctl to locked_ioctl
>> or old_ioctl to make sure we didn't miss any, and then mandate that this one
>> is only used when unlocked_ioctl is set to default_ioctl.
>
>
> I just looked at the patch in question and noted that the changelog
> is pretty high, but how could it be else.
> Actually it's not that large, but highly spread:
>
[Documentation/ arch/, drivers/, drivers/, and more drivers/, fs/,
include/, lib/, net/, sound/, virt/]
> 157 files changed, 372 insertions(+), 80 deletions(-)
>
>
> I wonder if we should actually just turn all these into unlocked_ioctl
> directly. And then bring a warn on ioctl, and finally schedule the removal
> of this callback.
A side note: A considerable portion of this particular commit in Arnd's
git actually does not deal with .ioctl->.unlocked_ioctl at all, but
purely with .llseek. Many(?) of these changes deal with .ioctl and
.llseek together. (Arnd also says so in the last paragraph of his
changelog.)
IOW there are less .ioctl implementations left than one could think from
a look at the diffstat.
--
Stefan Richter
-=====-==-=- -=-- ----=
http://arcgraph.de/sr/
On Thursday 01 April 2010, Stefan Richter wrote:
> >
> > I wonder if we should actually just turn all these into unlocked_ioctl
> > directly. And then bring a warn on ioctl, and finally schedule the removal
> > of this callback.
>
> A side note: A considerable portion of this particular commit in Arnd's
> git actually does not deal with .ioctl->.unlocked_ioctl at all, but
> purely with .llseek. Many(?) of these changes deal with .ioctl and
> .llseek together. (Arnd also says so in the last paragraph of his
> changelog.)
>
> IOW there are less .ioctl implementations left than one could think from
> a look at the diffstat.
Given our recent discussions on the llseek topic, it's probably better to
revert most of the changes that purely deal with llseek. My current idea
is to use an explicit default_llseek only if one of the following is given:
- we convert ioctl to unlocked_ioctl in the same file_operations, or
- the module uses the big kernel lock explicitly elsewhere.
Even then, there may be a number of cases where we can show it not
to be necessary, e.g. when the driver does not care about f_pos.
Concurrent llseek is racy by nature, so in most drivers, using the
BKL in llseek does not gain anything over using i_mutex.
Arnd
On Thu, Apr 01, 2010 at 02:45:32PM +0200, Arnd Bergmann wrote:
> On Thursday 01 April 2010, Stefan Richter wrote:
> > >
> > > I wonder if we should actually just turn all these into unlocked_ioctl
> > > directly. And then bring a warn on ioctl, and finally schedule the removal
> > > of this callback.
> >
> > A side note: A considerable portion of this particular commit in Arnd's
> > git actually does not deal with .ioctl->.unlocked_ioctl at all, but
> > purely with .llseek. Many(?) of these changes deal with .ioctl and
> > .llseek together. (Arnd also says so in the last paragraph of his
> > changelog.)
> >
> > IOW there are less .ioctl implementations left than one could think from
> > a look at the diffstat.
>
> Given our recent discussions on the llseek topic, it's probably better to
> revert most of the changes that purely deal with llseek. My current idea
> is to use an explicit default_llseek only if one of the following is given:
>
> - we convert ioctl to unlocked_ioctl in the same file_operations, or
> - the module uses the big kernel lock explicitly elsewhere.
>
> Even then, there may be a number of cases where we can show it not
> to be necessary, e.g. when the driver does not care about f_pos.
> Concurrent llseek is racy by nature, so in most drivers, using the
> BKL in llseek does not gain anything over using i_mutex.
>
> Arnd
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.
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.
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
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.
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
On Mon, Apr 12, 2010 at 07:34:17PM +0200, Arnd Bergmann wrote:
> > - 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 ;-)
Yes, it's not quite easily greppable. Making no seek allowed the
implicit default will fortunately allow us to get rid of that oddness.
> > - 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.
That's why it's last on the list.
> 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.
That plan sounds good to me.
> Should we also rename default_llseek to deprecated_llseek in the
> process, to go along with the approach for ioctl?
I wouldn't bother. If you can actually work on your plan default_llseek
should be gone soon enough.
On Tue, Apr 13, 2010 at 11:26:27AM +0200, Arnd Bergmann wrote:
> 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
Ok looks like a good plan then.
Thanks.