[patch 00/28] BKL removal queued patches

Arnd Bergmann · External Since: Oct 12, 2009 Posts: 10

On Saturday 10 October 2009, Thomas Gleixner wrote:
> There is nothing to protect inside nvram_llseek(), the file
> offset doesn't need to be protected and nvram_len is only
> initialized from an __init path.
>
> It's safe to remove the big kernel lock there.
>

The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
I guess it would be helpful to change that in the same series, so we
don't get the BKL back as soon as someone does a pushdown into the
remaining ioctl functions.

Arnd <><
--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Sun, Oct 11, 2009 at 11:50:24PM +0200, Arnd Bergmann wrote:
> On Sunday 11 October 2009, Frederic Weisbecker wrote:
> > Now I'm adding the ioctl() sites too:
> >
> > git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
> > 452
> >
> > Hehe Smile

>
> Not all of them, fortunately.
>
> There are various *_operations structures that have a .ioctl pointer.
> While there are a lot of struct file_operations with a locked .ioctl
> operation, stuff like block_device_operations does not hold the
> BKL in .ioctl but in .locked_ioctl.
>
> Arnd <><

Oh right. Thanks for the tip.

--
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/

Benjamin Herrenschmidt · External Since: Feb 16, 2005 Posts: 971

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (drivers-mac-ans-lcd-remove-bkl.patch)
> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
>
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
>
> Signed-off-by: Thomas Gleixner <tglx RemoveThis @linutronix.de>
> Cc: Benjamin Herrenschmidt <benh RemoveThis @kernel.crashing.org>
> Cc: linuxppc-dev RemoveThis @ozlabs.org
> ---

While I -do- have an ANS ... it's rusting in the back of my garage and I
really don't have time nor space to set it up and get it back to booting
shape Smile

(It's a pretty huge thing)

Patch looks good, so if it builds...

Acked-by: Benjamin Herrenschmidt <benh RemoveThis @kernel.crashing.org>

Cheers,
Ben.

--
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/

Arnd Bergmann · External Since: Oct 12, 2009 Posts: 10

On Saturday 10 October 2009, Thomas Gleixner wrote:
> 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.

The driver still holds the BKL in its ioctl method, which should be removed
as well if the above is true.

Arnd <><
--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Mon, Oct 12, 2009 at 12:25:05AM +0200, Arnd Bergmann wrote:
> On Monday 12 October 2009, Frederic Weisbecker wrote:
> > nvram_ioctl is a bkl locked ioctl, but it can be an unlocked ioctl.
> >
> > - part is provided by the user
> > - offset is provided by pmac_get_partition() which is safe as it only
> > touches nvram_partitions, an array inistialized on __init time and
> > read-only the rest of the time.
> > - nvram_sync() only relies on core99_nvram_sync() which checks
> > is_core_99, nvram_data, nvram_image. Those are variables initialized
> > on __init time only and their direct values are not touched further.
> > The rest modifies the nvram image header, protected by nv_lock
> > already.
> >
> > So it's safe to call nvram_ioctl without the big kernel lock held.
> >
> > Reported-by: Arnd Bergmann <arndbergmann.TakeThisOut@googlemail.com>
> > Signed-off-by: Frederic Weisbecker <fweisbec.TakeThisOut@gmail.com>
> > Cc: Thomas Gleixner <tglx.TakeThisOut@linutronix.de>
> > Cc: Ingo Molnar <mingo.TakeThisOut@elte.hu>
> > Cc: John Kacur <jkacur.TakeThisOut@redhat.com>
> > Cc: Sven-Thorsten Dietrich <sven.TakeThisOut@thebigcorporation.com>
> > Cc: Jonathan Corbet <corbet.TakeThisOut@lwn.net>
> > Cc: Alessio Igor Bogani <abogani.TakeThisOut@texware.it>
> > Cc: Arnd Bergmann <arndbergmann.TakeThisOut@googlemail.com>
> > Cc: Alan Cox <alan.TakeThisOut@lxorguk.ukuu.org.uk>
>
> Hmm, I wish I had not started using the gmail MTA for sending out
> mail, not that address is public forever.

Sorry, didn't know about that. I'm then taking Arnd Bergmann <arnd.TakeThisOut@arndb.de>
found in the MAINTAINER file for the v2 patch.

> > drivers/char/generic_nvram.c | 10 +++++-----
> > 1 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> > index c49200e..fd448aa 100644
> > --- a/drivers/char/generic_nvram.c
> > +++ b/drivers/char/generic_nvram.c
> > @@ -118,11 +118,11 @@ static int nvram_ioctl(struct inode *inode, struct file *file,
> > }
> >
> > const struct file_operations nvram_fops = {
> > - .owner = THIS_MODULE,
> > - .llseek = nvram_llseek,
> > - .read = read_nvram,
> > - .write = write_nvram,
> > - .ioctl = nvram_ioctl,
> > + .owner = THIS_MODULE,
> > + .llseek = nvram_llseek,
> > + .read = read_nvram,
> > + .write = write_nvram,
> > + .unlocked_ioctl = nvram_ioctl,
> > };
> >
> > static struct miscdevice nvram_dev = {
>
> The ioctl and unlocked_ioctl functions have a different signature, so you
> need to adapt that, not just rename it.

Hmm, that's the problem with this file I can't event build test. I really
should grab a cross compiler for powerpc targets.

>
> Not sure if we should do it in the same patch, but this driver should also
> have a compat_ioctl method pointing to the same function. The ioctl numbers
> in this driver are all 32/64 bit clean, but are not listed in fs/compat_ioctl.c,
> so adding a .compat_ioctl pointer is the easiest way to fix 32 bit userland
> accessing the device.
>
> Arnd <><

Added in my todolist then, for a saperate patch because it fixes another
issue.

Thanks!

--
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: Oct 12, 2009 Posts: 10

On Sunday 11 October 2009, Frederic Weisbecker wrote:
> Now I'm adding the ioctl() sites too:
>
> git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
> 452
>
> Hehe Smile

Not all of them, fortunately.

There are various *_operations structures that have a .ioctl pointer.
While there are a lot of struct file_operations with a locked .ioctl
operation, stuff like block_device_operations does not hold the
BKL in .ioctl but in .locked_ioctl.

Arnd <><
--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Sun, Oct 11, 2009 at 09:31:40PM +0200, Arnd Bergmann wrote:
> On Saturday 10 October 2009, Thomas Gleixner wrote:
> > There is nothing to protect inside nvram_llseek(), the file
> > offset doesn't need to be protected and nvram_len is only
> > initialized from an __init path.
> >
> > It's safe to remove the big kernel lock there.
> >
>
> The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
> I guess it would be helpful to change that in the same series, so we
> don't get the BKL back as soon as someone does a pushdown into the
> remaining ioctl functions.
>
> Arnd <><

Right!
I'll add that in a second patch.

I've completely forgotten this ioctl/unlocked_ioctl thing.

--
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/

Thomas Gleixner · External Since: May 14, 2006 Posts: 944

On Sun, 11 Oct 2009, Arnd Bergmann wrote:

> On Saturday 10 October 2009, Thomas Gleixner wrote:
> > 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.
>
> The driver still holds the BKL in its ioctl method, which should be removed
> as well if the above is true.

On my list already Smile

--
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/

Arnd Bergmann · External Since: Oct 12, 2009 Posts: 10

On Saturday 10 October 2009, Thomas Gleixner wrote:
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.

This only works as long as all misc drivers also don't need the BKL
in /their/ open methods. I believe that we did a pushdown into them
last year, but new ones may have crept up.

I guess we really should have kill this one off back then, but somehow
it was forgotten about.

Arnd <><
--
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/

Benjamin Herrenschmidt · External Since: Feb 16, 2005 Posts: 971

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (push-bkl-to-do-sysctl.patch)
> Push lock/unlock_kernel() into do_sysctl() and remove it from all call
> sites of do_sysctl().
>
> Signed-off-by: Thomas Gleixner <tglx.DeleteThis@linutronix.de>
> Cc: Tony Luck <tony.luck.DeleteThis@intel.com>
> Cc: Ralf Baechle <ralf.DeleteThis@linux-mips.org>
> Cc: Benjamin Herrenschmidt <benh.DeleteThis@kernel.crashing.org>
> Cc: Martin Schwidefsky <schwidefsky.DeleteThis@de.ibm.com>
> Cc: "David S. Miller" <davem.DeleteThis@davemloft.net>

For the powerpc parts,

Acked-by: Benjamin Herrenschmidt <benh.DeleteThis@kernel.crashing.org>

> ---
> arch/ia64/ia32/sys_ia32.c | 2 --
> arch/mips/kernel/linux32.c | 2 --
> arch/parisc/kernel/sys_parisc32.c | 2 --
> arch/powerpc/kernel/sys_ppc32.c | 2 --
> arch/s390/kernel/compat_linux.c | 2 --
> arch/sparc/kernel/sys_sparc32.c | 2 --
> arch/x86/ia32/sys_ia32.c | 2 --
> kernel/sysctl.c | 6 ++++--
> 8 files changed, 4 insertions(+), 16 deletions(-)
>
> Index: linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/ia64/ia32/sys_ia32.c
> +++ linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
> @@ -1670,10 +1670,8 @@ sys32_sysctl (struct sysctl32 __user *ar
> return -EFAULT;
>
> set_fs(KERNEL_DS);
> - lock_kernel();
> ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
> newvalp, (size_t) a32.newlen);
> - unlock_kernel();
> set_fs(old_fs);
>
> if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
> Index: linux-2.6-tip/arch/mips/kernel/linux32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/mips/kernel/linux32.c
> +++ linux-2.6-tip/arch/mips/kernel/linux32.c
> @@ -302,10 +302,8 @@ SYSCALL_DEFINE1(32_sysctl, struct sysctl
> oldlenp = (size_t __user *)addr;
> }
>
> - lock_kernel();
> error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
> oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
> - unlock_kernel();
> if (oldlenp) {
> if (!error) {
> if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/parisc/kernel/sys_parisc32.c
> +++ linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
> @@ -137,11 +137,9 @@ asmlinkage long sys32_sysctl(struct __sy
> oldlenp = (size_t *)addr;
> }
>
> - lock_kernel();
> error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
> (void __user *)(u64)tmp.oldval, oldlenp,
> (void __user *)(u64)tmp.newval, tmp.newlen);
> - unlock_kernel();
> if (oldlenp) {
> if (!error) {
> if (get_user(oldlen, (size_t *)addr)) {
> Index: linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/powerpc/kernel/sys_ppc32.c
> +++ linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
> @@ -555,11 +555,9 @@ asmlinkage long compat_sys_sysctl(struct
> return -EFAULT;
> }
>
> - lock_kernel();
> error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
> compat_ptr(tmp.oldval), oldlenp,
> compat_ptr(tmp.newval), tmp.newlen);
> - unlock_kernel();
> if (oldlenp) {
> if (!error) {
> if (get_user(oldlen, oldlenp) ||
> Index: linux-2.6-tip/arch/s390/kernel/compat_linux.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/s390/kernel/compat_linux.c
> +++ linux-2.6-tip/arch/s390/kernel/compat_linux.c
> @@ -562,10 +562,8 @@ asmlinkage long sys32_sysctl(struct __sy
> oldlenp = (size_t __user *)addr;
> }
>
> - lock_kernel();
> error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
> oldlenp, compat_ptr(tmp.newval), tmp.newlen);
> - unlock_kernel();
> if (oldlenp) {
> if (!error) {
> if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/sparc/kernel/sys_sparc32.c
> +++ linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
> @@ -627,14 +627,12 @@ asmlinkage long sys32_sysctl(struct __sy
> oldlenp = (size_t __user *)addr;
> }
>
> - lock_kernel();
> error = do_sysctl((int __user *)(unsigned long) tmp.name,
> tmp.nlen,
> (void __user *)(unsigned long) tmp.oldval,
> oldlenp,
> (void __user *)(unsigned long) tmp.newval,
> tmp.newlen);
> - unlock_kernel();
> if (oldlenp) {
> if (!error) {
> if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/x86/ia32/sys_ia32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/x86/ia32/sys_ia32.c
> +++ linux-2.6-tip/arch/x86/ia32/sys_ia32.c
> @@ -477,10 +477,8 @@ asmlinkage long sys32_sysctl(struct sysc
> return -EFAULT;
>
> set_fs(KERNEL_DS);
> - lock_kernel();
> ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
> newvalp, (size_t) a32.newlen);
> - unlock_kernel();
> set_fs(old_fs);
>
> if (oldvalp && put_user(oldlen, (int __user *)compat_ptr(a32.oldlenp)))
> Index: linux-2.6-tip/kernel/sysctl.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/sysctl.c
> +++ linux-2.6-tip/kernel/sysctl.c
> @@ -1848,6 +1848,8 @@ int do_sysctl(int __user *name, int nlen
> return -EFAULT;
> }
>
> + lock_kernel();
> +
> for (head = sysctl_head_next(NULL); head;
> head = sysctl_head_next(head)) {
> error = parse_table(name, nlen, oldval, oldlenp,
> @@ -1858,6 +1860,8 @@ int do_sysctl(int __user *name, int nlen
> break;
> }
> }
> +
> + unlock_kernel();
> return error;
> }
>
> @@ -1873,10 +1877,8 @@ SYSCALL_DEFINE1(sysctl, struct __sysctl_
> if (error)
> goto out;
>
> - lock_kernel();
> error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
> tmp.newval, tmp.newlen);
> - unlock_kernel();
> out:
> return error;
> }
>

--
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/

Pavel Machek · External Since: Nov 18, 2004 Posts: 1302

Hi!!

> > > drivers/staging/android/binder.c
> > > drivers/staging/dream/qdsp5/audio_aac.c
> > > drivers/staging/dream/qdsp5/audio_evrc.c
> > > drivers/staging/dream/qdsp5/audio_in.c
> > > drivers/staging/dream/qdsp5/audio_out.c
> > > drivers/staging/dream/qdsp5/snd.c
> > > drivers/staging/dream/smd/smd_qmi.c
> > > drivers/staging/panel/panel.c
>
> More sigh.

I guess you can just pretend these do not exist.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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/

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

From d3da91fe463a0799911ed7c826ec133c38c0a18b Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur RemoveThis @redhat.com>
Date: Wed, 21 Oct 2009 22:46:30 +0200
Subject: [PATCH] RNG: Explicitly set llseek = no_llseek after the BKL is removed.

Now that we've removed the BKL, lets explicitly set llseek to no_llseek
since it is not used here.

Signed-off-by: John Kacur <jkacur RemoveThis @redhat.com>
---
drivers/char/hw_random/core.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 75fb859..9cfd338 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -147,6 +147,7 @@ static const struct file_operations rng_chrdev_ops = {
.owner = THIS_MODULE,
.open = rng_dev_open,
.read = rng_dev_read,
+ .llseek = no_llseek,
};

static struct miscdevice rng_miscdev = {
--
1.6.0.6

--
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/

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

From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur.TakeThisOut@redhat.com>
Date: Wed, 21 Oct 2009 23:01:12 +0200
Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

Now that we've removed the BKL here, let's explicitly set lleek to no_llseek

Signed-off-by: John Kacur <jkacur.TakeThisOut@redhat.com>
---
drivers/macintosh/ans-lcd.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
index 4ae8ec9..a1a1bde 100644
--- a/drivers/macintosh/ans-lcd.c
+++ b/drivers/macintosh/ans-lcd.c
@@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
.write = anslcd_write,
.unlocked_ioctl = anslcd_ioctl,
.open = anslcd_open,
+ .llseedk = no_llseek,
};

static struct miscdevice anslcd_dev = {
--
1.6.0.6

--
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/

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

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.

Signed-off-by: John Kacur <jkacur DeleteThis @redhat.com>
---
drivers/char/efirtc.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

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= {
--
1.6.0.6

--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur.DeleteThis@redhat.com>
> Date: Wed, 21 Oct 2009 23:01:12 +0200
> Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
>
> Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
>
> Signed-off-by: John Kacur <jkacur.DeleteThis@redhat.com>
> ---
> drivers/macintosh/ans-lcd.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> index 4ae8ec9..a1a1bde 100644
> --- a/drivers/macintosh/ans-lcd.c
> +++ b/drivers/macintosh/ans-lcd.c
> @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
> .write = anslcd_write,
> .unlocked_ioctl = anslcd_ioctl,
> .open = anslcd_open,
> + .llseedk = no_llseek,

llseedk? Smile

Should we better pushdown default_llseek to every to every
file operations that don't implement llseek?
I don't know how many of them don't implement llseek() though.

That said we can't continue anymore with this default attribution
of default_llseek() on new fops.

--
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 Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> > From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur.RemoveThis@redhat.com>
> > Date: Wed, 21 Oct 2009 23:01:12 +0200
> > Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> >
> > Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> >
> > Signed-off-by: John Kacur <jkacur.RemoveThis@redhat.com>
> > ---
> > drivers/macintosh/ans-lcd.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> > index 4ae8ec9..a1a1bde 100644
> > --- a/drivers/macintosh/ans-lcd.c
> > +++ b/drivers/macintosh/ans-lcd.c
> > @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
> > .write = anslcd_write,
> > .unlocked_ioctl = anslcd_ioctl,
> > .open = anslcd_open,
> > + .llseedk = no_llseek,
>
>
> llseedk? Smile

>
>
> Should we better pushdown default_llseek to every to every
> file operations that don't implement llseek?
> I don't know how many of them don't implement llseek() though.
>
> That said we can't continue anymore with this default attribution
> of default_llseek() on new fops.
>

If you don't explicitly set it to no_llseek, you automatically get the
default_llseek, which uses the BKL. So if your driver doesn't need it, it
is best to explicitly set it to no_llseek.

There is also a generic_file_llseek_unlocked, somewhat analogous to the
unlocked_ioctls that you can use if you don't need to provide a full
llseek yourself.
--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > Should we better pushdown default_llseek to every to every
> > file operations that don't implement llseek?
> > I don't know how many of them don't implement llseek() though.
> >
> > That said we can't continue anymore with this default attribution
> > of default_llseek() on new fops.
> >
>
> If you don't explicitly set it to no_llseek, you automatically get the
> default_llseek, which uses the BKL. So if your driver doesn't need it, it
> is best to explicitly set it to no_llseek.

Sure, that's the right thing to do.

> There is also a generic_file_llseek_unlocked, somewhat analogous to the
> unlocked_ioctls that you can use if you don't need to provide a full
> llseek yourself.

No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
depending on the context is the right thing to do.

What I'm wondering about concerns the future code that will have
no llsek() implemented in their fops.

We can't continue to use default_llseek() for future code unless we
want to continue these post reviews and fixes forever.

--
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/

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

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > > Should we better pushdown default_llseek to every to every
> > > file operations that don't implement llseek?
> > > I don't know how many of them don't implement llseek() though.
> > >
> > > That said we can't continue anymore with this default attribution
> > > of default_llseek() on new fops.
> > >
> >
> > If you don't explicitly set it to no_llseek, you automatically get the
> > default_llseek, which uses the BKL. So if your driver doesn't need it, it
> > is best to explicitly set it to no_llseek.
>
>
> Sure, that's the right thing to do.
>
>
> > There is also a generic_file_llseek_unlocked, somewhat analogous to the
> > unlocked_ioctls that you can use if you don't need to provide a full
> > llseek yourself.
>
>
> No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> depending on the context is the right thing to do.
>
> What I'm wondering about concerns the future code that will have
> no llsek() implemented in their fops.
>
> We can't continue to use default_llseek() for future code unless we
> want to continue these post reviews and fixes forever.
>

I'm thinking that the simplier approach, would be to make the
default_llseek the unlocked one. Then you only have to audit the drivers
that have the BKL - ie the ones we are auditing anyway, and explicitly set
them to the bkl locked llseek.

There might be a hidden interaction though between the non-unlocked
variety of ioctls and default llseek though.
--
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/

Frederic Weisbecker · External Since: Jan 17, 2009 Posts: 267

On Wed, Oct 21, 2009 at 11:53:21PM +0200, John Kacur wrote:
> > No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> > depending on the context is the right thing to do.
> >
> > What I'm wondering about concerns the future code that will have
> > no llsek() implemented in their fops.
> >
> > We can't continue to use default_llseek() for future code unless we
> > want to continue these post reviews and fixes forever.
> >
>
> I'm thinking that the simplier approach, would be to make the
> default_llseek the unlocked one. Then you only have to audit the drivers
> that have the BKL - ie the ones we are auditing anyway, and explicitly set
> them to the bkl locked llseek.
>
> There might be a hidden interaction though between the non-unlocked
> variety of ioctls and default llseek though.

I fear that won't work because the bkl in default_llseek() does not
only synchronizes with others uses of the bkl in a driver, it also
synchronizes lseek() itself.

As an example offset change is not atomic. This is a long long, so
updating its value is not atomic in 32 bits archs.

--
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/

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

On Thursday 22 October 2009, Frederic Weisbecker wrote:
> > I'm thinking that the simplier approach, would be to make the
> > default_llseek the unlocked one. Then you only have to audit the drivers
> > that have the BKL - ie the ones we are auditing anyway, and explicitly set
> > them to the bkl locked llseek.
> >
> > There might be a hidden interaction though between the non-unlocked
> > variety of ioctls and default llseek though.
>
>
> I fear that won't work because the bkl in default_llseek() does not
> only synchronizes with others uses of the bkl in a driver, it also
> synchronizes lseek() itself.
>
> As an example offset change is not atomic. This is a long long, so
> updating its value is not atomic in 32 bits archs.

A late follow-up on this one:

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.

Arnd <><
--
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/