[PATCH 4/6] procfs: Use generic_file

Frederic Weisbecker

unread,

Mar 30, 2010, 2:30:01 AM3/30/10

to

/proc/vmcore has no llseek and then falls down to use default_llseek.
This is racy against read_vmcore() that directly manipulates fpos
but it doesn't hold the bkl there so using it in llseek doesn't
protect anything.

Let's use generic_file_llseek() instead.

Signed-off-by: Frederic Weisbecker <fwei...@gmail.com>
Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: John Kacur <jka...@redhat.com>
Cc: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Cc: Al Viro <vi...@ZenIV.linux.org.uk>
---
fs/proc/vmcore.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 0872afa..f942ecb 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -162,6 +162,7 @@ static ssize_t read_vmcore(struct file *file, char __user *buffer,

static const struct file_operations proc_vmcore_operations = {
.read = read_vmcore,
+ .lseek = generic_file_llseek,
};

static struct vmcore* __init get_new_element(void)
--
1.6.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Frederic Weisbecker

unread,

Mar 30, 2010, 2:30:02 AM3/30/10

to

From: Arnd Bergmann <ar...@arndb.de>

We don't use the BKL elsewhere, so use generic_file_llseek
so we can avoid default_llseek taking the BKL.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
[restore proc_fdinfo_file_operations as non-seekable]
Signed-off-by: Frederic Weisbecker <fwei...@gmail.com>

Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: John Kacur <jka...@redhat.com>
Cc: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Cc: Al Viro <vi...@ZenIV.linux.org.uk>
---

fs/proc/base.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a731084..95d91cf 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -728,6 +728,7 @@ out_no_task:

static const struct file_operations proc_info_file_operations = {
.read = proc_info_read,
+ .llseek = generic_file_llseek,
};

static int proc_single_show(struct seq_file *m, void *v)
@@ -985,6 +986,7 @@ out_no_task:

static const struct file_operations proc_environ_operations = {
.read = environ_read,
+ .llseek = generic_file_llseek,
};

static ssize_t oom_adjust_read(struct file *file, char __user *buf,
@@ -1058,6 +1060,7 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
static const struct file_operations proc_oom_adjust_operations = {
.read = oom_adjust_read,
.write = oom_adjust_write,
+ .llseek = generic_file_llseek,
};

#ifdef CONFIG_AUDITSYSCALL
@@ -1129,6 +1132,7 @@ out_free_page:
static const struct file_operations proc_loginuid_operations = {
.read = proc_loginuid_read,
.write = proc_loginuid_write,
+ .llseek = generic_file_llseek,
};

static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
@@ -1149,6 +1153,7 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,

static const struct file_operations proc_sessionid_operations = {
.read = proc_sessionid_read,
+ .llseek = generic_file_llseek,
};
#endif

@@ -1200,6 +1205,7 @@ static ssize_t proc_fault_inject_write(struct file * file,
static const struct file_operations proc_fault_inject_operations = {
.read = proc_fault_inject_read,
.write = proc_fault_inject_write,
+ .llseek = generic_file_llseek,
};
#endif

@@ -1941,7 +1947,7 @@ static ssize_t proc_fdinfo_read(struct file *file, char __user *buf,
}

static const struct file_operations proc_fdinfo_file_operations = {
- .open = nonseekable_open,
+ .open = nonseekable_open,
.read = proc_fdinfo_read,
};

@@ -2225,6 +2231,7 @@ out_no_task:
static const struct file_operations proc_pid_attr_operations = {
.read = proc_pid_attr_read,
.write = proc_pid_attr_write,
+ .llseek = generic_file_llseek,
};

static const struct pid_entry attr_dir_stuff[] = {
@@ -2345,6 +2352,7 @@ static ssize_t proc_coredump_filter_write(struct file *file,
static const struct file_operations proc_coredump_filter_operations = {
.read = proc_coredump_filter_read,
.write = proc_coredump_filter_write,
+ .llseek = generic_file_llseek,
};
#endif

Frederic Weisbecker

unread,

Mar 30, 2010, 2:30:02 AM3/30/10

to

There are no more users of procfs that implement the ioctl
callback. Drop the bkl from this path and warn on any use
of this callback.

Signed-off-by: Frederic Weisbecker <fwei...@gmail.com>
Cc: Arnd Bergmann <ar...@arndb.de>

Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: John Kacur <jka...@redhat.com>
Cc: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Cc: Al Viro <vi...@ZenIV.linux.org.uk>
---

fs/proc/inode.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 445a02b..afcda85 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
if (rv == -ENOIOCTLCMD)
rv = -EINVAL;
} else if (ioctl) {
- lock_kernel();
+ WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
+ "%pf will be called without the Bkl held\n", ioctl);
rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
- unlock_kernel();
}

pde_users_dec(pde);

Frederic Weisbecker

unread,

Mar 30, 2010, 2:30:02 AM3/30/10

to

No need to hold the bkl to seek here, none of the other
fops callbacks use it.

Use generic_file_llseek explicitly.

Signed-off-by: Frederic Weisbecker <fwei...@gmail.com>
Cc: Arnd Bergmann <ar...@arndb.de>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: Andrew Morton <ak...@linux-foundation.org>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: John Kacur <jka...@redhat.com>
Cc: KAMEZAWA Hiroyuki <kamezaw...@jp.fujitsu.com>
Cc: Al Viro <vi...@ZenIV.linux.org.uk>
---

fs/proc/kmsg.c | 1 +

1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/proc/kmsg.c b/fs/proc/kmsg.c
index cfe90a4..bd4b5a7 100644
--- a/fs/proc/kmsg.c
+++ b/fs/proc/kmsg.c
@@ -53,6 +53,7 @@ static const struct file_operations proc_kmsg_operations = {
.poll = kmsg_poll,
.open = kmsg_open,
.release = kmsg_release,
+ .llseek = generic_file_llseek,
};

static int __init proc_kmsg_init(void)

Alexey Dobriyan

unread,

Mar 30, 2010, 2:50:02 AM3/30/10

to

On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fwei...@gmail.com> wrote:
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -728,6 +728,7 @@ out_no_task:
>
> static const struct file_operations proc_info_file_operations = {
> .read = proc_info_read,
> + .llseek = generic_file_llseek,

There is no warning for default default_llseek case.
This should be done same way as proc ioctls.

Alexey Dobriyan

unread,

Mar 30, 2010, 2:50:02 AM3/30/10

to

On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fwei...@gmail.com> wrote:

> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> if (rv == -ENOIOCTLCMD)
> rv = -EINVAL;
> } else if (ioctl) {
> - lock_kernel();
> + WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> + "%pf will be called without the Bkl held\n", ioctl);
> rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> - unlock_kernel();

Then delete the branch.
Or go through formal feature-removal procedure.

Frederic Weisbecker

unread,

Mar 30, 2010, 3:00:02 AM3/30/10

to

On Tue, Mar 30, 2010 at 09:40:24AM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fwei...@gmail.com> wrote:
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -728,6 +728,7 @@ out_no_task:
> >
> > static const struct file_operations proc_info_file_operations = {
> > .read = proc_info_read,
> > + .llseek = generic_file_llseek,
>
> There is no warning for default default_llseek case.
> This should be done same way as proc ioctls.

I don't think we should. We have overriden the llseek for
the procfs users located in the proc core (just fs/proc)
but we haven't touched all of the external users, and since
there are hundreds of them, I guess a lot don't implement
llseek.

We would need to first override those that are visible upstream
and warn for the further ones after this step only.

Frederic Weisbecker

unread,

Mar 30, 2010, 3:10:01 AM3/30/10

to

On Tue, Mar 30, 2010 at 09:38:11AM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fwei...@gmail.com> wrote:
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> > if (rv == -ENOIOCTLCMD)
> > rv = -EINVAL;
> > } else if (ioctl) {
> > - lock_kernel();
> > + WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> > + "%pf will be called without the Bkl held\n", ioctl);
> > rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> > - unlock_kernel();
>
> Then delete the branch.
> Or go through formal feature-removal procedure.

I thought about it. I even started to write something in this
feature-removal file but realized that I can't remove the
.ioctl() callback from file operations. We still need to check
the user hasn't made the mistake of implementing it.

What I can plan as a feature removal, though, is to keep the warning
but don't actually call the ioctl.

Arnd Bergmann

unread,

Mar 30, 2010, 6:40:02 AM3/30/10

to

On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> No need to hold the bkl to seek here, none of the other
> fops callbacks use it.
>
> Use generic_file_llseek explicitly.
>

Acked-by: Arnd Bergmann <ar...@arndb.de>

Arnd Bergmann

unread,

Mar 30, 2010, 6:40:02 AM3/30/10

to

On Tuesday 30 March 2010, Frederic Weisbecker wrote:

> /proc/vmcore has no llseek and then falls down to use default_llseek.
> This is racy against read_vmcore() that directly manipulates fpos
> but it doesn't hold the bkl there so using it in llseek doesn't
> protect anything.
>
> Let's use generic_file_llseek() instead.
>

Acked-by: Arnd Bergmann <ar...@arndb.de>

Arnd Bergmann

unread,

Mar 30, 2010, 6:40:02 AM3/30/10

to

On Tuesday 30 March 2010, Frederic Weisbecker wrote:
> On Tue, Mar 30, 2010 at 09:38:11AM +0300, Alexey Dobriyan wrote:
> > On Tue, Mar 30, 2010 at 9:20 AM, Frederic Weisbecker <fwei...@gmail.com> wrote:
> > > --- a/fs/proc/inode.c
> > > +++ b/fs/proc/inode.c
> > > @@ -231,9 +231,9 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne
> > > if (rv == -ENOIOCTLCMD)
> > > rv = -EINVAL;
> > > } else if (ioctl) {
> > > - lock_kernel();
> > > + WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, "
> > > + "%pf will be called without the Bkl held\n", ioctl);
> > > rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg);
> > > - unlock_kernel();
> >
> > Then delete the branch.
> > Or go through formal feature-removal procedure.
>
>
> I thought about it. I even started to write something in this
> feature-removal file but realized that I can't remove the
> .ioctl() callback from file operations. We still need to check
> the user hasn't made the mistake of implementing it.
>
> What I can plan as a feature removal, though, is to keep the warning
> but don't actually call the ioctl.

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 also remember going through procfs ioctl operations some time ago and finding
exactly three users, which I believe are the same ones that Frederic found.

Arnd

Frederic Weisbecker

unread,

Mar 31, 2010, 1:30:02 PM3/31/10

to

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:

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.

What do you think?

You plan looks good but I fear this actually carries the problem forward
in that we won't be able to remove .ioctl after that.

I can handle that if you agree.

Arnd Bergmann

unread,

Mar 31, 2010, 4:30:02 PM3/31/10

to

On Wednesday 31 March 2010 19:22:11 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:

<snip>

> 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.
>
> What do you think?

I don't think the warning helps all that much, at least not across an
entire release. We could leave it in for the merge window and fix all
users for -rc1, then submit a patch that kills everything that came
in during the merge window and remove it completely in -rc2.

Getting rid of ioctl completely is a lot of work though, covering the
entire lot of ~150 device drivers. I think the patch as is (or the
variant renaming .ioctl to .locked_ioctl) is far less work and has
less potential of introducing regressions.

> You plan looks good but I fear this actually carries the problem forward
> in that we won't be able to remove .ioctl after that.
>
> I can handle that if you agree.

I don't think we really need to get rid of it this soon in the obsolete
drivers, pushing down the BKL into an unlocked_ioctl function only slightly
shifts the problem around, since the driver still depends on the BKL then
and gets disabled if you build with CONFIG_BKL=n.

IMHO, a better use of your time would be to completely remove the BKL
along with the ioctl function from any of the drivers in this lists that
looks like it could be relevant to real users.

In the meantime, we can move the declaration of the .locked_ioctl callback
into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an
ioctl function that does not get called.

Another crazy idea I had was to simply turn the BKL into a regular mutex
as soon as we can show that all remaining users are of the non-recursive
kind and don't rely on the autorelease-on-sleep. Doing that would be
much easier without the pushdown into .unlocked_ioctl than it would be
with it.

Arnd

Arnd Bergmann

unread,

Mar 31, 2010, 5:10:02 PM3/31/10

to

On Wednesday 31 March 2010 22:21:23 Arnd Bergmann wrote:
> Another crazy idea I had was to simply turn the BKL into a regular mutex
> as soon as we can show that all remaining users are of the non-recursive
> kind and don't rely on the autorelease-on-sleep. Doing that would be
> much easier without the pushdown into .unlocked_ioctl than it would be
> with it.

I just looked at all the users of lock_kernel remaining with my patch
series. For 90% of them, it is completely obvious that they don't rely
on nested locking, and they very much look like they don't need the
autorelease either, because the BKL was simply pushed down into the
open, ioctl and llseek functions.

There are a few file systems (udf, ncpfs, autofs, coda, ...) and some
network protocols (appletalk, ipx, irnet and x25) for which it is not
obviously, though still quite likely, the case.

So we could actually remove the BKL recursion code soon, or even turn
all of it into a regular mutex, at least as an experimental option.

The recursive users that I've removed in my series are the block, tty,
input and sound subsystems, as well as the init code.

Frederic Weisbecker

unread,

Mar 31, 2010, 5:50:03 PM3/31/10

to

Hmm, yeah you're right actually. Since we have this CONFIG_BKL thing
plus a future check to prevent from people implementing new ioctl
(checking ioctl without default_ioctl), it's actually better than
a big pushdown as it's less invasive.

> In the meantime, we can move the declaration of the .locked_ioctl callback
> into an #ifdef CONFIG_BKL, to make sure nobody builds a driver with an
> ioctl function that does not get called.

Ok, now how to get this all merged? A single monolithic patch is probably
not appropriate.

The simplest is to have a single branch with the default_ioctl implemented,
and then attributed to drivers in a set cut by subsystems/drivers. And
push the whole for the next -rc1.

The other solution is to push default_ioctl for this release and get
the driver changes to each concerned tree. That said, I suspect a good
part of them are unmaintained, hence the other solution looks better
to me.

Hmm?

Alan Cox

unread,

Mar 31, 2010, 6:00:02 PM3/31/10

to

> The recursive users that I've removed in my series are the block, tty,
> input and sound subsystems, as well as the init code.

There are some very subtle recursive cases in the tty code - hangup
triggered close and consoles being an absolute gem I've just had to debug
in my lock removal bits so far...

Alan

Frederic Weisbecker

unread,

Mar 31, 2010, 6:00:02 PM3/31/10

to

On Wed, Mar 31, 2010 at 11:04:30PM +0200, Arnd Bergmann wrote:
> On Wednesday 31 March 2010 22:21:23 Arnd Bergmann wrote:
> > Another crazy idea I had was to simply turn the BKL into a regular mutex
> > as soon as we can show that all remaining users are of the non-recursive
> > kind and don't rely on the autorelease-on-sleep. Doing that would be
> > much easier without the pushdown into .unlocked_ioctl than it would be
> > with it.
>
> I just looked at all the users of lock_kernel remaining with my patch
> series. For 90% of them, it is completely obvious that they don't rely
> on nested locking, and they very much look like they don't need the
> autorelease either, because the BKL was simply pushed down into the
> open, ioctl and llseek functions.
>
> There are a few file systems (udf, ncpfs, autofs, coda, ...) and some
> network protocols (appletalk, ipx, irnet and x25) for which it is not
> obviously, though still quite likely, the case.
>
> So we could actually remove the BKL recursion code soon, or even turn
> all of it into a regular mutex, at least as an experimental option.
>
> The recursive users that I've removed in my series are the block, tty,
> input and sound subsystems, as well as the init code.

This is a solution that has been tried more than once already. But Linus
has told he wouldn't pull something that turns the bkl into a mutex or a
semaphore.

Plus it's quite hard to tell that it does or not auto-release somewhere
This is often something you can really spot on runtime or on small path
only.

The simple fact the bkl is not always a leaf lock makes it need the
auto-release, otherwise you experience very bad unexpected lock
dependencies.

Arnd Bergmann

unread,

Apr 1, 2010, 5:10:01 AM4/1/10

to

On Wednesday 31 March 2010, Alan Cox wrote:
> > The recursive users that I've removed in my series are the block, tty,
> > input and sound subsystems, as well as the init code.
>
> There are some very subtle recursive cases in the tty code - hangup
> triggered close and consoles being an absolute gem I've just had to debug
> in my lock removal bits so far...

Yes, I've seen some of them. What I meant above is that with
CONFIG_TTY_MUTEX=y, the TTY code no longer uses the BKL in a
nested way, and quite likely no either code does either.

The TTY code with my patch now has tty_lock() for all cases that
I concluded are never nested in another tty_lock, and tty_lock_nested()
for those I did not understand or that I know they are nested (the
latter type usually comes with a comment). The only difference
between the two is a WARN_ON(tty_locked()) in tty_lock, so we can
see where the analysis was wrong.

Arnd

Stefan Richter

unread,

Apr 1, 2010, 7:40:01 AM4/1/10

to

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/

Arnd Bergmann

unread,

Apr 1, 2010, 7:40:01 AM4/1/10

to

On Wednesday 31 March 2010, Frederic Weisbecker wrote:
> This is a solution that has been tried more than once already. But Linus
> has told he wouldn't pull something that turns the bkl into a mutex or a
> semaphore.

Ok. Starting from the same observation of simplicity in the remaining code,
we should also be able to find a semi-automatic way of turning the BKL usage
in these drivers into a per-module mutex.

> Plus it's quite hard to tell that it does or not auto-release somewhere
> This is often something you can really spot on runtime or on small path
> only.

Well, the autorelease by itself is not needed anywhere. What is needed
is the consequence of autorelease avoiding AB-BA type deadlocks.

> The simple fact the bkl is not always a leaf lock makes it need the
> auto-release, otherwise you experience very bad unexpected lock
> dependencies.

I'm arguing that we can probably show the BKL to be the outermost
lock for the majority of the remaining drivers, which only get it
from their open(), ioctl() or llseek() functions, which are all
called without any locks held. If the BKL is a regular mutex, lockdep
should warn of the other cases.

Arnd

Arnd Bergmann

unread,

Apr 1, 2010, 8:50:03 AM4/1/10

to

On Wednesday 31 March 2010, Frederic Weisbecker wrote:

I don't care much about the unmaintained parts, we can always have a
tree collecting all the patches for those drivers and merge it in -rc1.

I'd say the nicest way would be to get Linus to merge the patch
below now, so we can start queuing stuff in maintainer trees on top
of it, and check against linux-next what is still missing and push
all of them from our branch.

Arnd

---
Subject: [PATCH] introduce CONFIG_BKL and default_ioctl

This is a preparation for the removal of the big kernel lock that
introduces new interfaces for device drivers still using it.

We can start marking those device drivers as 'depends on CONFIG_BKL'
now, and make that symbol optional later, when the point has come
at which we are able to build a kernel without the BKL.

Similarly, device drivers that currently make use of the implicit
BKL locking around the ioctl function can now get annotated by
changing

.ioctl = foo_ioctl,

to

.locked_ioctl = foo_ioctl,
.unlocked_ioctl = default_ioctl,

As soon as no driver remains using the old ioctl callback, it can
get removed.

Signed-off-by: Arnd Bergmann <ar...@arndb.de>
---
fs/ioctl.c | 22 ++++++++++++++++++++++
include/linux/fs.h | 3 +++
include/linux/smp_lock.h | 4 ++++
lib/Kconfig.debug | 10 ++++++++++
4 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..52c2698 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
return error;
}

+#ifdef CONFIG_BKL
+/*
+ * default_ioctl - call unlocked_ioctl with BKL held
+ *
+ * Setting only the the ioctl operation but not unlocked_ioctl will become
+ * invalid in the future, all drivers that are not converted to unlocked_ioctl
+ * should set .unlocked_ioctl = default_ioctl now.
+ */
+long default_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+ int error = -ENOTTY;
+ if (filp->f_op->locked_ioctl) {
+ lock_kernel();
+ error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
+ filp, cmd, arg);
+ unlock_kernel();
+ }
+ return error;
+}
+EXPORT_SYMBOL_GPL(default_ioctl);
+#endif
+
static int ioctl_fibmap(struct file *filp, int __user *p)
{
struct address_space *mapping = filp->f_mapping;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 10b8ded..93afdb4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1492,6 +1492,9 @@ struct file_operations {
int (*readdir) (struct file *, void *, filldir_t);
unsigned int (*poll) (struct file *, struct poll_table_struct *);
int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_BKL
+ int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+#endif
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
index 2ea1dd1..9cec605 100644
--- a/include/linux/smp_lock.h
+++ b/include/linux/smp_lock.h
@@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
#define kernel_locked() 1

#endif /* CONFIG_LOCK_KERNEL */
+
+loff_t default_llseek(struct file *file, loff_t offset, int origin);
+long default_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+
#endif /* __LINUX_SMPLOCK_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1fafb4b..a4852d6 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -444,6 +444,16 @@ config DEBUG_MUTEXES
This feature allows mutex semantics violations to be detected and
reported.

+config BKL
+ def_bool y
+ help
+ This is the traditional lock that is used in old code instead
+ of proper locking. All drivers that use the BKL should depend
+ on this symbol.
+ This configuration option will become user-selectable in the
+ future, as soon as it is possible to build a kernel without
+ it.
+
config DEBUG_LOCK_ALLOC
bool "Lock debugging: detect incorrect freeing of live locks"
depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT

Arnd Bergmann

unread,

Apr 1, 2010, 8:50:02 AM4/1/10

to

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

John Kacur

unread,

Apr 1, 2010, 12:40:03 PM4/1/10

to

----- "Arnd Bergmann" <ar...@arndb.de> wrote:

> On Wednesday 31 March 2010 22:21:23 Arnd Bergmann wrote:
> > Another crazy idea I had was to simply turn the BKL into a regular
> mutex
> > as soon as we can show that all remaining users are of the
> non-recursive
> > kind and don't rely on the autorelease-on-sleep. Doing that would
> be
> > much easier without the pushdown into .unlocked_ioctl than it would
> be
> > with it.
>
> I just looked at all the users of lock_kernel remaining with my patch
> series. For 90% of them, it is completely obvious that they don't
> rely
> on nested locking, and they very much look like they don't need the
> autorelease either, because the BKL was simply pushed down into the
> open, ioctl and llseek functions.
>
> There are a few file systems (udf, ncpfs, autofs, coda, ...) and some
> network protocols (appletalk, ipx, irnet and x25) for which it is not
> obviously, though still quite likely, the case.
>
> So we could actually remove the BKL recursion code soon, or even turn
> all of it into a regular mutex, at least as an experimental option.

Well, that would be quite similar to what we do in real-time with the
"Big Kernel Semaphore". However, Linus didn't want that pushed into
mainstream. As an experimental tree it's fine, but we're really stuck
with removing the BKL one by one until it's gone.

Stefan Richter

unread,

Apr 3, 2010, 2:00:03 PM4/3/10

to

Arnd Bergmann wrote:
> Subject: [PATCH] introduce CONFIG_BKL and default_ioctl

[...]

> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
> return error;
> }
>
> +#ifdef CONFIG_BKL
> +/*
> + * default_ioctl - call unlocked_ioctl with BKL held

[...]

> + error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
> + filp, cmd, arg);

Typo. default_ioctl - call locked_ioctl with BKL held
--
Stefan Richter
-=====-==-=- -=-- ---==
http://arcgraph.de/sr/

Frederic Weisbecker

unread,

Apr 10, 2010, 11:30:03 AM4/10/10

to

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.

Frederic Weisbecker

unread,

Apr 10, 2010, 12:20:02 PM4/10/10

to

Al, we would like to make this for 2.6.34, so that we can start
converting the drivers that use bkl'ed ioctl to this new scheme
in the relevant maintainers trees.

Can we get your ack?

Thanks.

Frederic Weisbecker

unread,

Apr 10, 2010, 12:20:01 PM4/10/10

to

On Thu, Apr 01, 2010 at 02:42:38PM +0200, Arnd Bergmann 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.

Frederic Weisbecker

unread,

Apr 10, 2010, 12:30:02 PM4/10/10

to

On Thu, Apr 01, 2010 at 02:42:38PM +0200, Arnd Bergmann wrote:

Al, we would like to make this for 2.6.34, so that we can start
converting the drivers that use bkl'ed ioctl to this new scheme
in the relevant maintainers trees.

Can we get your ack?

Thanks.

--

Christoph Hellwig

unread,

Apr 11, 2010, 9:10:02 AM4/11/10

to

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.

Arnd Bergmann

unread,

Apr 12, 2010, 11:10:02 AM4/12/10

to

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

Arnd Bergmann

unread,

Apr 12, 2010, 1:40:02 PM4/12/10

to

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

Frederic Weisbecker

unread,

Apr 12, 2010, 6:00:03 PM4/12/10

to

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.

Arnd Bergmann

unread,

Apr 13, 2010, 5:30:02 AM4/13/10

to

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

Christoph Hellwig

unread,

Apr 13, 2010, 2:10:02 PM4/13/10

to

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.

Frederic Weisbecker

unread,

Apr 13, 2010, 4:50:03 PM4/13/10

to

Ok looks like a good plan then.

Thanks.

[PATCH 4/6] procfs: Use generic_file_llseek in /proc/vmcore

Frederic Weisbecker

Frederic Weisbecker

Frederic Weisbecker

Frederic Weisbecker

Alexey Dobriyan

Alexey Dobriyan

Frederic Weisbecker

Frederic Weisbecker

Arnd Bergmann

Arnd Bergmann

Arnd Bergmann

Frederic Weisbecker

Arnd Bergmann

Arnd Bergmann

Frederic Weisbecker

Alan Cox

Frederic Weisbecker

Arnd Bergmann

Stefan Richter

Arnd Bergmann

Arnd Bergmann

Arnd Bergmann

John Kacur

Stefan Richter

Frederic Weisbecker

Frederic Weisbecker

Frederic Weisbecker

Frederic Weisbecker

Christoph Hellwig

Arnd Bergmann

Arnd Bergmann

Frederic Weisbecker

Arnd Bergmann

Christoph Hellwig

Frederic Weisbecker