[patch 00/28] BKL removal queued patches

David Miller · External Since: Sep 25, 2006 Posts: 1156

From: Thomas Gleixner <tglx RemoveThis @linutronix.de>
Date: Sat, 10 Oct 2009 15:36:48 -0000

> cycle_kernel_lock() was added with the BKL pushdown. The rio driver
> indeed needs that because riowd_device is initialized after
> misc_register(). So an open(), write/ioctl() which happens to get
> between misc_register returning and riowd_device initialization would
> dereference a NULL pointer.
>
> Move riowd_device initialization before misc_register() and get rid of
> cycle_kernel_lock().
>
> Signed-off-by: Thomas Gleixner <tglx RemoveThis @linutronix.de>

Also applied to sparc-next-2.6, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

David Miller · External Since: Sep 25, 2006 Posts: 1156

From: Thomas Gleixner <tglx.DeleteThis@linutronix.de>
Date: Sat, 10 Oct 2009 15:36:39 -0000

> commit ab772027 (sparc: arch/sparc/kernel/apc.c to unlocked_ioctl)
> added lock/unlock_kernel() to the apc ioctl code.
>
> The code needs no serialization at all. Neither put/get_user nor the
> read/write access to the sbus devices require it. Remove BKL.
>
> cycle_kernel_lock() was added during the big BKL pushdown. It should
> ensure the serializiation against driver init code. In this case there
> is nothing to serialize. Remove it as well.
>
> Signed-off-by: Thomas Gleixner <tglx.DeleteThis@linutronix.de>

Sorry, forgot to explicitly say I applied this to sparc-next-2.6, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andrew Morton · External Since: Feb 02, 2007 Posts: 2303

On Wed, 21 Oct 2009 23:13:26 +0200 (CEST)
John Kacur <jkacur.DeleteThis@redhat.com> wrote:

> >From e1b7175258b33da3b0564ef04a0b1956f04f0cc7 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur.DeleteThis@redhat.com>
> Date: Wed, 21 Oct 2009 23:10:30 +0200
> Subject: [PATCH] rtc: Explicitly set llseek to no_llseek
>
> Now that we've removed the BKL here, lets explicitly set llseek to no_llseek
> since the default llseek is not used here.
>

I don't understand.

>
> diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
> index 26a47dc..53c524e 100644
> --- a/drivers/char/efirtc.c
> +++ b/drivers/char/efirtc.c
> @@ -285,6 +285,7 @@ static const struct file_operations efi_rtc_fops = {
> .unlocked_ioctl = efi_rtc_ioctl,
> .open = efi_rtc_open,
> .release = efi_rtc_close,
> + .llseek = no_llseek,
> };
>
> static struct miscdevice efi_rtc_dev= {

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

John Kacur · External Since: Jul 23, 2009 Posts: 23

On Tue, 3 Nov 2009, Andrew Morton wrote:

> On Wed, 21 Oct 2009 23:13:26 +0200 (CEST)
> John Kacur <jkacur.TakeThisOut@redhat.com> wrote:
>
> > >From e1b7175258b33da3b0564ef04a0b1956f04f0cc7 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur.TakeThisOut@redhat.com>
> > Date: Wed, 21 Oct 2009 23:10:30 +0200
> > Subject: [PATCH] rtc: Explicitly set llseek to no_llseek
> >
> > Now that we've removed the BKL here, lets explicitly set llseek to no_llseek
> > since the default llseek is not used here.
> >
>
> I don't understand.
>
> >
> > diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
> > index 26a47dc..53c524e 100644
> > --- a/drivers/char/efirtc.c
> > +++ b/drivers/char/efirtc.c
> > @@ -285,6 +285,7 @@ static const struct file_operations efi_rtc_fops = {
> > .unlocked_ioctl = efi_rtc_ioctl,
> > .open = efi_rtc_open,
> > .release = efi_rtc_close,
> > + .llseek = no_llseek,
> > };
> >
> > static struct miscdevice efi_rtc_dev= {
>
> What has this change to do with the BKL?

The default_llseek function still contains the BKL.
When we are auditing code to see if we can remove the BKL, this is one of
the hidden considerations we need to take into account.
i.e., is there syncronization between code that has the BKL and llseek.

At the same time we remove the BKL it would be a good idea to do indicate
when no llseek function is required, so we don't have to revisit this code
again, when we are trying to determine if we can remove the BKL from
the default_llseek.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Christoph Hellwig · External Since: Dec 05, 2004 Posts: 277

On Mon, Nov 02, 2009 at 04:51:52PM +0100, Arnd Bergmann wrote:
> I looked at what places in the code manipulate file->f_pos directly
> and found that almost all the uses in driver code are broken because
> they don't take any locks. Most of them are in driver specific
> lseek operations. Others are in read/write methods and are even
> more broken because the change gets immediately overwritten by
> vfs_read/vfs_write when the driver method returns.
>
> IMHO, we should complete the pushdown into all ioctl methods
> that John and Thomas have started working on, fixing the lseek
> methods in those files we touch.
>
> When that is done, all interaction between default_llseek and
> driver locking has to be with explicit calls to lock_kernel
> in those drivers, so we can reasonably well script the search
> for drivers needing the BKL in llseek: everyhing that
> a) defines file_operations without an llseek function,
> b) touches f_pos somewhere, and
> c) calls lock_kernel() somewhere
> That should only be a small number and when they are fixed,
> we can remove default_llseek and instead call generic_file_llseek
> for any file operation without a separate llseek method.

As mentioned before making generic_file_llseek the new default is
probably a bad idea. The majority of our file_operations instances
don't actually support seeking, so no_llseek should become the new
default if you spend some effort on converting things. Anything that
wants to allow seeking will have to set a llseek method. This also
mirrors what we do for other file operations. None of the major ones
has a non-trivial default, it's either silently succeeding for a
selected few like open or release or returning an error for operatings
that actually do something like read and write.

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

Arnd Bergmann · External Since: May 03, 2004 Posts: 429

On Monday 16 November 2009, Christoph Hellwig wrote:
> As mentioned before making generic_file_llseek the new default is
> probably a bad idea. The majority of our file_operations instances
> don't actually support seeking, so no_llseek should become the new
> default if you spend some effort on converting things. Anything that
> wants to allow seeking will have to set a llseek method. This also
> mirrors what we do for other file operations. None of the major ones
> has a non-trivial default, it's either silently succeeding for a
> selected few like open or release or returning an error for operatings
> that actually do something like read and write.

Ok, good point.

Do you think we should also prevent pread/pwrite for devices without
an llseek operation, like nonseekable_open does? I guess that would
be consistent.

Then there is the point that (I forgot who) brought up that changing
code to do no_llseek is actually an ABI change. Even if the file
position is never used anywhere, some random user application might
expect a chardev not to return an error when its llseek method is
called, resulting in regressions.

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