Re: [PATCH 6/6] procfs: Kill the bkl in ioctl

[Frederic Weisbecker <fweisbec@gmail.com>]

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.