Killing the Big Kernel Lock

I've spent some time continuing the work of the people on Cc and many others to remove the big kernel lock from Linux and I now have bkl-removal branch in my git tree at git://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
that lets me run a kernel on my quad-core machine with the only users of the BKL being mostly obscure device driver modules.

The oldest patch in this series is roughly eight years old and is Willy's patch to remove the BKL from fs/locks.c, and I took a series of patches from Jan that removes it from most of the VFS.

The other non-obvious changes are:

- all file operations that either have an .ioctl method or do not have their own .llseek method used to implicitly require the BKL. I've changed that so they need to explicitly set .llseek = default_llseek, .unlocked_ioctl = default_ioctl, and changed all the code that either has supplied a .ioctl method or looks like it needs the BKL somewhere else, meaning the default_llseek function might actually do something.

- The block layer now has a global bkldev_mutex that is used in all block drivers in place of the BKL. The only recursive instance of the BKL was __blkdev_get(), which is now called with the blkdev_mutex held instead of grabbing the BKL. This has some possible performance implications that need to be looked into.

- The init/main.c code no longer take the BKL. I figured that this was completely unnecessary because there is no other code running at the same time that takes the BKL.

- The most invasive change is in the TTY layer, which has a new global mutex (sorry!). I know that Alan has plans of his own to remove the BKL from this subsystem, so my patches may not go anywhere, but they seem to work fine for me.
I've called the new lock the 'Big TTY Mutex' (BTM), a name that probably makes more sense if you happen to speak German.
The basic idea here is to make recursive locking and the release-on-sleep explicit, so every mutex_lock, wait_event, workqueue_flush and schedule in the TTY layer now explicitly releases the BTM before blocking.

- All drivers that still require the BKL are now listed as 'depends on BKL' in Kconfig, and you can set that symbol to 'y', 'm' or 'n'. If the lock itself is a module, only other modules can use it, and /proc/modules will tell you exactly which ones those are. I've thought about adding a module_init function in that module that will taint the kernel, but so far I haven't done that.

- Included is a debugfs file that gives statistics over the BKL usage from early boot on. This is now obsolete and will not get merged, but I'm including it for reference.

Frederic has volunteered to help merging all of this upstream, which I very much welcome. The shape that the tree is in now is very inconsistent, especially some of the bits at the end are a bit dodgy and all of it needs more testing.

I've built-tested an allmodconfig kernel with CONFIG_BKL disabled on x86_64, i386, powerpc64, powerpc32, s390 and arm to make sure I catch all the modules that depend on BKL, and I've been running various versions of this tree on my desktop machine over the last few weeks while adding stuff.

Arnd

---

Arnd Bergmann (44):
input: kill BKL, fix input_open_file locking
ptrace: kill BKL
procfs: kill BKL in llseek
random: forbid llseek on random chardev x86/microcode: use nonseekable_open
perf_event: use nonseekable_open
dm: use nonseekable_open
vgaarb: use nonseekable_open
kvm: don't require BKL
nvram: kill BKL
do_coredump: do not take BKL
hpet: kill BKL, add compat_ioctl
proc/pci: kill BKL
autofs/autofs4: move compat_ioctl handling into fs usb/mon: kill BKL usage
fat: push down BKL
sunrpc: push down BKL
pcmcia: push down BKL
vfs: kill BKL in default_llseek
BKL: introduce CONFIG_BKL.
bkl-removal: make fops->ioctl and default_llseek optional
x86: update defconfig to CONFIG_BKL=m
bkl removal: make unlocked_ioctl mandatory
bkl removal: use default_llseek in code that uses the BKL BKL removal: mark remaining users as 'depends on BKL'
tty: replace BKL with a new tty_lock
tty: make atomic_write_lock release tty_lock
tty: make tty_port->mutex nest under tty_lock
tty: make termios mutex nest under tty_lock
tty: make ldisc_mutex nest under tty_lock
tty: never hold tty_lock() while getting tty_mutex
ppp: use big tty mutex
tty: release tty lock when blocking
tty: implement BTM as mutex instead of BKL
briq_panel: do not use BTM
affs: remove leftover unlock_kernel
kvm: don't require BKL
block: replace BKL with global mutex
init: kill BKL usage
debug: instrument big kernel lock
BKL removal: make the BKL modular

Matthew Wilcox (1):
[RFC] Remove BKL from fs/locks.c

Jan Blunck (19):
JFS: Free sbi memory in error path
BKL: Explicitly add BKL around get_sb/fill_super
BKL: Remove outdated comment and include
BKL: Remove BKL from Amiga FFS
BKL: Remove BKL from BFS
BKL: Remove BKL from CifsFS
BKL: Remove BKL from ext3 fill_super()
BKL: Remove BKL from ext3_put_super() and ext3_remount()
BKL: Remove BKL from ext4 filesystem
BKL: Remove smp_lock.h from exofs
BKL: Remove BKL from HFS
BKL: Remove BKL from HFS+
BKL: Remove BKL from JFS
BKL: Remove BKL from NILFS2
BKL: Remove BKL from NTFS
BKL: Remove BKL from cgroup
BKL: Remove BKL from do_new_mount()
ext2: Add ext2_sb_info s_lock spinlock
BKL: Remove BKL from ext2 filesystem

Hi Arnd,

Interesting work. For the drivers/infiniband part, it seems maybe all these drivers should be using no_llseek instead of default_llseek? (or is it better style to use nonseekable_open()?) Certainly as far as I can tell, nothing in drivers/infiniband pays any attention to f_pos.

Also, is there a reason why you add "#include <linux/smp_lock.h>" to all the files where you also do ".llseek = default_llseek"?

In any case I can at least take care of the llseek stuff for 2.6.35.

- R.

Roland Dreier  <[email protected]>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

no_llseek makes it clear that you don't want the default_llseek semantics, while nonseekable_open also prevents pread/pwrite. Ideally, I'd just use both.

There is a small chance that a random user space application actually tries to seek on the device (e.g. SEEK_END) and expects a zero return value, so when in doubt, I converted everything to default_llseek instead of no_llseek, just so I can be sure I don't change the semantics.

The last patch in the series moves the default_llseek and default_ioctl function into the same loadable module that contains the BKL itself. Moving the declarations into the respective header seemed appropriate, but it could also stay in a VFS header if people prefer that.

Ok, thanks!

Arnd

OK, I added the following to my tree, currently queued in my for-next branch for 2.6.35:

IB: Explicitly rule out llseek to avoid BKL in default_llseek()

Several RDMA user-access drivers have file_operations structures with no .llseek method set. None of the drivers actually do anything with f_pos, so this means llseek is essentially a NOP, instead of returning an error as leaving other file_operations methods unimplemented would do. This is mostly harmless, except that a NULL .llseek means that default_llseek() is used, and this function grabs the BKL, which we would like to avoid.

Since llseek does nothing useful on these files, we would like it to return an error to userspace instead of silently grabbing the BKL and succeeding. For nearly all of the file types, we take the belt-and-suspenders approach of setting the .llseek method to no_llseek and also calling nonseekable_open(); the exception is the uverbs_event files, which are created with anon_inode_getfile(), which already sets f_mode the same way as nonseekable_open() would.

This work is motivated by Arnd Bergmann's bkl-removal tree.

---
 drivers/infiniband/core/ucm.c         |    3 ++-
 drivers/infiniband/core/ucma.c        |    4 +++-
 drivers/infiniband/core/user_mad.c    |   12 ++++++++----
 drivers/infiniband/core/uverbs_main.c |   11 +++++++----
 4 files changed, 20 insertions(+), 10 deletions(-)

index 017d6e2..7ef3954 100644
@@ -1180,7 +1180,7 @@ static int ib_ucm_open(struct inode *inode, struct file *filp)
 	file->filp = filp;
 	file->device = container_of(inode->i_cdev, struct ib_ucm_device, cdev);
 
-	return 0;
+	return nonseekable_open(inode, filp);
 }
 
 static int ib_ucm_close(struct inode *inode, struct file *filp)

@@ -1228,6 +1228,7 @@ static const struct file_operations ucm_fops = {
 	.release = ib_ucm_close,
 	.write	 = ib_ucm_write,
 	.poll    = ib_ucm_poll,
+	.llseek	 = no_llseek,
 };
 
 static ssize_t show_ibdev(struct device *dev, struct device_attribute *attr,

index b2e16c3..09d4a3b 100644
@@ -1219,7 +1219,8 @@ static int ucma_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = file;
 	file->filp = filp;
-	return 0;
+
+	return nonseekable_open(inode, filp);
 }
 
 static int ucma_close(struct inode *inode, struct file *filp)

@@ -1249,6 +1250,7 @@ static const struct file_operations ucma_fops = {
 	.release = ucma_close,
 	.write	 = ucma_write,
 	.poll    = ucma_poll,
+	.llseek	 = no_llseek,
 };
 
 static struct miscdevice ucma_misc = {

index 04b585e..2bb9703 100644
@@ -780,7 +780,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 {
 	struct ib_umad_port *port;
 	struct ib_umad_file *file;
-	int ret = 0;
+	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
 	if (port)

@@ -813,6 +813,8 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 
 	list_add_tail(&file->port_list, &port->file_list);
 
+	ret = nonseekable_open(inode, filp);
+
 out:
 	mutex_unlock(&port->file_mutex);
 	return ret;

@@ -865,7 +867,8 @@ static const struct file_operations umad_fops = {
 	.compat_ioctl	= ib_umad_compat_ioctl,
 #endif
 	.open		= ib_umad_open,
-	.release	= ib_umad_close
+	.release	= ib_umad_close,
+	.llseek		= no_llseek,
 };
 
 static int ib_umad_sm_open(struct inode *inode, struct file *filp)

@@ -902,7 +905,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = port;
 
-	return 0;
+	return nonseekable_open(inode, filp);
 
 fail:
 	kref_put(&port->umad_dev->ref, ib_umad_release_dev);

@@ -932,7 +935,8 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 static const struct file_operations umad_sm_fops = {
 	.owner	 = THIS_MODULE,
 	.open	 = ib_umad_sm_open,
-	.release = ib_umad_sm_close
+	.release = ib_umad_sm_close,
+	.llseek	 = no_llseek,
 };
 
 static struct ib_client umad_client = {

index 1444f95..a16a91e 100644
@@ -385,7 +385,8 @@ static const struct file_operations uverbs_event_fops = {
 	.read	 = ib_uverbs_event_read,
 	.poll    = ib_uverbs_event_poll,
 	.release = ib_uverbs_event_close,
-	.fasync  = ib_uverbs_event_fasync
+	.fasync  = ib_uverbs_event_fasync,
+	.llseek	 = no_llseek,
 };
 
 void ib_uverbs_comp_handler(struct ib_cq *cq, void *cq_context)

@@ -639,7 +640,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = file;
 
-	return 0;
+	return nonseekable_open(inode, filp);
 
 err_module:
 	module_put(dev->ib_dev->owner);

@@ -667,7 +668,8 @@ static const struct file_operations uverbs_fops = {
 	.owner	 = THIS_MODULE,
 	.write	 = ib_uverbs_write,
 	.open	 = ib_uverbs_open,
-	.release = ib_uverbs_close
+	.release = ib_uverbs_close,
+	.llseek	 = no_llseek,
 };
 
 static const struct file_operations uverbs_mmap_fops = {

@@ -675,7 +677,8 @@ static const struct file_operations uverbs_mmap_fops = {
 	.write	 = ib_uverbs_write,
 	.mmap    = ib_uverbs_mmap,
 	.open	 = ib_uverbs_open,
-	.release = ib_uverbs_close
+	.release = ib_uverbs_close,
+	.llseek	 = no_llseek,
 };
 
 static struct ib_client uverbs_client = {

1.7.0.3


-- 
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Chuckle (I don't but I looked it up)

I'm not sure if that is actually the path of sanity (yours at least), nor the right way to whack the other BKL users whose use is horrible but essentially private.

It would be nice to get the other bits in first removing BKL from most of the kernel and building kernels which are non BKL except for the tty layer. That (after Ingo's box from hell has run it a bit) would reasonably test the assertion that the tty layer has no BKL requirements that are driven by external to tty layer code.

That to me would test the biggest question of all and be a reasonably good base from which to then either apply the tty BTM patches or attack the problem properly with the BKL localised to one subtree.

Alan

Yes, we can do that by applying all patches except 'tty: implement BTM as mutex instead of BKL', which is the only one in the tty section of my series that should really change the behaviour. Building a kernel with all other BKL users gone currently implies disabling usbcore, videodev, soundcore, i4l and capi, as well as a large number of obsolete device drivers.

The only ones that I can imagine still interacting with the tty code are the ISDN drivers, and even those look pretty unlikely.

We could also make the 'tty: implement BTM as mutex instead of BKL' patch a config option that makes it possible to test it out some more while conservative users just continue to get the BKL semantics.

Arnd

Very nice work!

How about going one step further:

- remove CONFIG_BKL altogether
- remove all the remains of the BKL code in lib/kernel_lock.c and kernel/sched.c
- turn lock_kernel() into a WARN_ONCE() and unlock_kernel() into a NOP.
- ...
- Celebrate! :-)

Ingo

<looks inside ptrace.c>

Seems that there might be a few tricksy bits in here. Please do push at least the non-obvious parts out to the relevant people.

It'd be interesting to see the overall diffstat?

Sure, that is certainly the plan.

Regarding the ptrace bits, this is one of a handful of places where the BKL was put in by someone a really long time ago but with the rest of the series applied, it becomes evident that there is nothing whatsoever that it serializes with, so removing the BKL here does not make the situation worse. It could still be a bug that needs to be fixed by adding a new serialization method no matter if the BKL is there or not.

Let me give you three separate diffstats. One is for the trivial pushdown of the BKL into all the ioctl and llseek functions as well as marking all remaining users as 'depends on BKL' in Kconfig, the second one is for the TTY layer conversion to the Big TTY Mutex and the third one is all the rest.

Arnd

---
Big TTY Mutex conversion:

 drivers/char/Makefile           |    2 +
 drivers/char/amiserial.c        |   16 ++--
 drivers/char/cyclades.c         |   20 ++--
 drivers/char/epca.c             |    4 +-
 drivers/char/isicom.c           |   10 +-
 drivers/char/istallion.c        |   20 ++--
 drivers/char/mxser.c            |   10 +-
 drivers/char/n_hdlc.c           |   16 ++--
 drivers/char/n_r3964.c          |   10 +-
 drivers/char/pty.c              |    8 +-
 drivers/char/riscom8.c          |    8 +-
 drivers/char/rocket.c           |    8 +-
 drivers/char/serial167.c        |    4 +-
 drivers/char/specialix.c        |   10 +-
 drivers/char/stallion.c         |   12 ++--
 drivers/char/sx.c               |   12 ++--
 drivers/char/synclink.c         |   10 ++-
 drivers/char/synclink_gt.c      |    8 +-
 drivers/char/synclinkmp.c       |   12 ++--
 drivers/char/tty_buffer.c       |    2 +-
 drivers/char/tty_io.c           |  123 +++++++++++++++------------
 drivers/char/tty_ioctl.c        |   36 ++++----
 drivers/char/tty_ldisc.c        |   53 +++++++-----
 drivers/char/tty_lock.c         |  100 ++++++++++++++++++++++
 drivers/char/tty_port.c         |    6 +-
 drivers/char/vc_screen.c        |    4 +-
 drivers/char/vt.c               |    4 +-
 drivers/char/vt_ioctl.c         |   12 ++--
 drivers/isdn/i4l/isdn_common.c  |   20 ++--
 drivers/isdn/i4l/isdn_tty.c     |    8 +-
 drivers/net/irda/irtty-sir.c    |    5 +-
 drivers/net/ppp_generic.c       |   29 +++---
 drivers/serial/68360serial.c    |    4 +-
 drivers/serial/crisv10.c        |    8 +-
 drivers/serial/serial_core.c    |   42 +++++-----
 drivers/staging/strip/strip.c   |    2 +-
 drivers/usb/class/cdc-acm.c     |    2 +-
 drivers/usb/serial/usb-serial.c |   18 ++--
 drivers/video/console/vgacon.c  |    4 +-
 include/linux/init_task.h       |    1 +
 include/linux/sched.h           |    1 +
 include/linux/tty.h             |  180 +++++++++++++++++++++++++++++++++++++++
 kernel/fork.c                   |    1 +

ioctl/llseek pushdown, Kconfig:

 43 files changed, 590 insertions(+), 275 deletions(-)
 Documentation/DocBook/kernel-hacking.tmpl   |    2 +-
 Documentation/filesystems/vfs.txt           |    3 +-
 arch/arm/kernel/etm.c                       |    1 -
 arch/cris/arch-v10/drivers/ds1302.c         |    3 -
 arch/cris/arch-v10/drivers/gpio.c           |    2 -
 arch/cris/arch-v10/drivers/i2c.c            |    2 -
 arch/cris/arch-v10/drivers/pcf8563.c        |    3 -
 arch/cris/arch-v10/drivers/sync_serial.c    |    4 +-
 arch/cris/arch-v32/drivers/cryptocop.c      |    4 +-
 arch/cris/arch-v32/drivers/i2c.c            |    2 -
 arch/cris/arch-v32/drivers/mach-a3/gpio.c   |    2 -
 arch/cris/arch-v32/drivers/mach-fs/gpio.c   |    2 -
 arch/cris/arch-v32/drivers/pcf8563.c        |    5 +-
 arch/cris/arch-v32/drivers/sync_serial.c    |    4 +-
 arch/ia64/kernel/perfmon.c                  |    2 -
 arch/ia64/sn/kernel/sn2/sn_hwperf.c         |    2 -
 arch/m68k/bvme6000/rtc.c                    |    2 -
 arch/m68k/mvme16x/rtc.c                     |    2 -
 arch/powerpc/platforms/iseries/Kconfig      |    1 -
 arch/s390/Kconfig                           |    1 -
 arch/um/drivers/harddog_kern.c              |    2 -
 arch/um/drivers/hostaudio_kern.c            |    3 -
 arch/um/drivers/mmapper_kern.c              |    3 -
 arch/x86/Kconfig                            |    3 +-
 arch/x86/kvm/Kconfig                        |    1 -
 drivers/block/DAC960.c                      |    3 +-
 drivers/block/Kconfig                       |    6 +-
 drivers/block/paride/pg.c                   |    2 -
 drivers/block/paride/pt.c                   |    2 -
 drivers/block/pktcdvd.c                     |    3 -
 drivers/char/Kconfig                        |   25 +-
 drivers/char/apm-emulation.c                |    2 -
 drivers/char/applicom.c                     |    2 -
 drivers/char/ds1302.c                       |    1 -
 drivers/char/ds1620.c                       |    2 -
 drivers/char/dtlk.c                         |    2 -
 drivers/char/generic_nvram.c                |    2 -
 drivers/char/genrtc.c                       |    2 -
 drivers/char/i8k.c                          |    2 -
 drivers/char/ip2/ip2main.c                  |    1 -
 drivers/char/ipmi/Kconfig                   |    2 -
 drivers/char/ipmi/ipmi_devintf.c            |    2 -
 drivers/char/ipmi/ipmi_watchdog.c           |    2 -
 drivers/char/istallion.c                    |    1 -
 drivers/char/lp.c                           |    1 -
 drivers/char/mmtimer.c                      |    1 -
 drivers/char/nwflash.c                      |    1 -
 drivers/char/pcmcia/Kconfig                 |    4 +-
 drivers/char/raw.c                          |    4 -
 drivers/char/rio/rio_linux.c                |    1 -
 drivers/char/stallion.c                     |    1 -
 drivers/char/sx.c                           |    1 -
 drivers/char/uv_mmtimer.c                   |    1 -
 drivers/char/viotape.c                      |    1 -
 drivers/crypto/Kconfig                      |    2 +-
 drivers/firewire/Kconfig                    |    1 -
 drivers/firewire/core-cdev.c                |    2 -
 drivers/gpu/drm/Kconfig                     |    5 +-
 drivers/gpu/drm/i810/i810_dma.c             |    2 -
 drivers/gpu/drm/i830/i830_dma.c             |    2 -
 drivers/hid/Kconfig                         |    2 +-
 drivers/hid/usbhid/Kconfig                  |    2 +-
 drivers/hid/usbhid/hiddev.c                 |    3 +-
 drivers/hwmon/Kconfig                       |    2 +-
 drivers/hwmon/fschmd.c                      |    2 -
 drivers/ide/Kconfig                         |    1 -
 drivers/ide/ide-tape.c                      |    1 -
 drivers/ieee1394/Kconfig                    |    6 +-
 drivers/ieee1394/dv1394.c                   |    2 -
 drivers/ieee1394/raw1394.c                  |    2 -
 drivers/ieee1394/video1394.c                |    4 +-
 drivers/infiniband/Kconfig                  |    6 +-
 drivers/infiniband/core/ucm.c               |    2 -
 drivers/infiniband/core/ucma.c              |    2 -
 drivers/infiniband/core/user_mad.c          |    3 -
 drivers/infiniband/core/uverbs_main.c       |   11 +-
 drivers/input/joystick/Kconfig              |    2 +-
 drivers/input/misc/Kconfig                  |   13 +-
 drivers/input/misc/hp_sdc_rtc.c             |    2 -
 drivers/input/misc/uinput.c                 |    1 -
 drivers/input/mouse/Kconfig                 |    4 +-
 drivers/input/serio/Kconfig                 |    1 -
 drivers/input/tablet/Kconfig                |    8 +-
 drivers/input/touchscreen/Kconfig           |    2 +-
 drivers/isdn/Kconfig                        |    2 -
 drivers/isdn/capi/Kconfig                   |    2 +-
 drivers/isdn/capi/capi.c                    |    1 -
 drivers/isdn/divert/divert_procfs.c         |    2 -
 drivers/isdn/hardware/eicon/Kconfig         |    2 +-
 drivers/isdn/hysdn/Kconfig                  |    2 +-
 drivers/isdn/i4l/Kconfig                    |    1 -
 drivers/isdn/i4l/isdn_common.c              |    1 -
 drivers/isdn/mISDN/Kconfig                  |    1 -
 drivers/isdn/mISDN/timerdev.c               |    3 -
 drivers/macintosh/Kconfig                   |    7 +-
 drivers/macintosh/ans-lcd.c                 |    2 -
 drivers/macintosh/nvram.c                   |    2 -
 drivers/macintosh/via-pmu.c                 |    2 -
 drivers/media/Kconfig                       |    5 +-
 drivers/media/dvb/bt8xx/Kconfig             |    2 +-
 drivers/media/dvb/bt8xx/dst_ca.c            |    1 -
 drivers/media/dvb/dvb-core/dmxdev.c         |    3 -
 drivers/media/dvb/dvb-core/dvb_ca_en50221.c |    3 -
 drivers/media/dvb/dvb-core/dvb_frontend.c   |    5 +-
 drivers/media/dvb/dvb-core/dvb_net.c        |    3 -
 drivers/media/dvb/firewire/Kconfig          |    2 +-
 drivers/media/dvb/firewire/firedtv-ci.c     |    3 -
 drivers/media/dvb/ttpci/Kconfig             |    2 +-
 drivers/media/dvb/ttpci/av7110.c            |    3 -
 drivers/media/dvb/ttpci/av7110_av.c         |    5 -
 drivers/media/dvb/ttpci/av7110_ca.c         |    3 -
 drivers/media/video/Kconfig                 |   13 +-
 drivers/media/video/cpia.c                  |    2 -
 drivers/media/video/v4l2-dev.c              |    2 -
 drivers/message/fusion/Kconfig              |    2 +-
 drivers/message/i2o/Kconfig                 |    2 +-
 drivers/misc/Kconfig                        |    2 +-
 drivers/mtd/Kconfig                         |    1 -
 drivers/mtd/mtdchar.c                       |    1 -
 drivers/mtd/ubi/Kconfig                     |    2 +-
 drivers/mtd/ubi/cdev.c                      |    2 -
 drivers/net/Kconfig                         |    1 -
 drivers/net/appletalk/Kconfig               |    1 -
 drivers/net/ppp_generic.c                   |    4 +-
 drivers/net/wan/Kconfig                     |    2 +-
 drivers/net/wireless/Kconfig                |    4 +-
 drivers/net/wireless/airo.c                 |    9 -
 drivers/net/wireless/ray_cs.c               |    3 -
 drivers/pci/hotplug/Kconfig                 |    2 +-
 drivers/pcmcia/Kconfig                      |    3 +-
 drivers/platform/x86/Kconfig                |    1 -
 drivers/pnp/isapnp/Kconfig                  |    2 +-
 drivers/rtc/Kconfig                         |    1 -
 drivers/rtc/rtc-m41t80.c                    |    1 -
 drivers/s390/block/Kconfig                  |    2 +-
 drivers/s390/char/Kconfig                   |    2 +-
 drivers/s390/char/fs3270.c                  |    1 -
 drivers/s390/char/tape_char.c               |    2 +-
 drivers/s390/cio/chsc_sch.c                 |    2 -
 drivers/s390/crypto/zcrypt_api.c            |    1 -
 drivers/s390/scsi/zfcp_cfdc.c               |    2 -
 drivers/sbus/char/envctrl.c                 |    1 -
 drivers/sbus/char/openprom.c                |    1 -
 drivers/scsi/3w-9xxx.c                      |    2 -
 drivers/scsi/3w-sas.c                       |    2 -
 drivers/scsi/3w-xxxx.c                      |    2 -
 drivers/scsi/Kconfig                        |   28 +-
 drivers/scsi/aacraid/linit.c                |    1 -
 drivers/scsi/dpt_i2o.c                      |    2 -
 drivers/scsi/gdth.c                         |    2 -
 drivers/scsi/megaraid.c                     |    2 -
 drivers/scsi/megaraid/Kconfig.megaraid      |    6 +-
 drivers/scsi/megaraid/megaraid_mm.c         |    2 -
 drivers/scsi/megaraid/megaraid_sas.c        |    1 -
 drivers/scsi/mpt2sas/Kconfig                |    2 +-
 drivers/scsi/mpt2sas/mpt2sas_ctl.c          |    1 -
 drivers/scsi/osd/Kconfig                    |    2 +-
 drivers/scsi/osd/osd_uld.c                  |    2 -
 drivers/scsi/osst.c                         |    2 -
 drivers/scsi/pmcraid.c                      |    2 -
 drivers/scsi/st.c                           |    1 -
 drivers/spi/spidev.c                        |    2 -
 drivers/staging/comedi/comedi_fops.c        |    2 -
 drivers/staging/dream/pmem.c                |    3 -
 drivers/staging/dream/qdsp5/audio_aac.c     |    2 -
 drivers/staging/dream/qdsp5/audio_mp3.c     |    2 -
 drivers/staging/poch/poch.c                 |    3 -
 drivers/staging/sep/sep_driver.c            |    2 -
 drivers/staging/vme/devices/vme_user.c      |    2 -
 drivers/telephony/Kconfig                   |    2 +-
 drivers/telephony/ixj.c                     |    1 -
 drivers/usb/Kconfig                         |    2 +-
 drivers/usb/class/Kconfig                   |    1 -
 drivers/usb/class/usblp.c                   |    2 -
 drivers/usb/gadget/Kconfig                  |    2 -
 drivers/usb/gadget/printer.c                |    1 -
 drivers/usb/host/Kconfig                    |    2 +-
 drivers/usb/misc/Kconfig                    |    8 +-
 drivers/usb/misc/idmouse.c                  |    2 -
 drivers/usb/misc/iowarrior.c                |    1 -
 drivers/usb/misc/rio500.c                   |    1 -
 drivers/usb/misc/sisusbvga/Kconfig          |    2 +-
 fs/adfs/Kconfig                             |    1 -
 fs/afs/Kconfig                              |    1 -
 fs/autofs/Kconfig                           |    1 -
 fs/autofs/root.c                            |    1 -
 fs/autofs4/Kconfig                          |    1 -
 fs/autofs4/dev-ioctl.c                      |    2 -
 fs/btrfs/super.c                            |    1 -
 fs/coda/Kconfig                             |    1 -
 fs/coda/pioctl.c                            |    3 -
 fs/coda/psdev.c                             |    2 -
 fs/ecryptfs/Kconfig                         |    1 -
 fs/ecryptfs/file.c                          |    2 -
 fs/ecryptfs/miscdev.c                       |    2 -
 fs/fat/Kconfig                              |    3 -
 fs/freevxfs/Kconfig                         |    1 -
 fs/hfsplus/Kconfig                          |    1 -
 fs/hfsplus/dir.c                            |    2 -
 fs/hfsplus/inode.c                          |    2 -
 fs/hpfs/Kconfig                             |    1 -
 fs/ioctl.c                                  |   11 +-
 fs/isofs/Kconfig                            |    1 -
 fs/jffs2/Kconfig                            |    1 -
 fs/ncpfs/Kconfig                            |    1 -
 fs/ncpfs/dir.c                              |    2 -
 fs/ncpfs/file.c                             |    1 -
 fs/nfs/Kconfig                              |    2 +-
 fs/nfsd/Kconfig                             |    1 -
 fs/ocfs2/Kconfig                            |    1 -
 fs/qnx4/Kconfig                             |    1 -
 fs/read_write.c                             |   34 +
 fs/reiserfs/Kconfig                         |    1 -
 fs/smbfs/Kconfig                            |    1 -
 fs/smbfs/dir.c                              |    2 -
 fs/smbfs/file.c                             |    1 -
 fs/squashfs/Kconfig                         |    1 -
 fs/udf/Kconfig                              |    1 -
 fs/udf/dir.c                                |    2 -
 fs/udf/file.c                               |    1 -
 fs/ufs/Kconfig                              |    2 +-
 include/linux/fs.h                          |    5 +
 kernel/power/Kconfig                        |    2 +-
 lib/Kconfig.debug                           |    2 +-
 lib/kernel_lock.c                           |   37 +-
 net/bluetooth/hidp/Kconfig                  |    1 -
 net/ipx/Kconfig                             |    1 -
 net/irda/Kconfig                            |    2 +-
 net/irda/irnet/Kconfig                      |    2 +-
 net/socket.c                                |    1 -
 net/sunrpc/Kconfig                          |    4 +-
 net/wanrouter/Kconfig                       |    2 +-
 net/x25/Kconfig                             |    2 +-
 sound/Kconfig                               |    2 +-
 sound/core/control.c                        |    2 -
 sound/core/oss/pcm_oss.c                    |    2 -
 sound/core/pcm_native.c                     |    2 -
 sound/core/seq/seq_clientmgr.c              |    2 -
 sound/oss/au1550_ac97.c                     |   30 +-
 sound/oss/dmasound/dmasound_core.c          |    2 -
 sound/oss/msnd_pinnacle.c                   |    2 -
 sound/oss/sh_dac_audio.c                    |    3 -
 sound/oss/swarm_cs4297a.c                   |    3 -
 sound/oss/vwsnd.c                           |    2 -
 sound/soc/soc-core.c                        |    2 -
 virt/kvm/kvm_main.c                         |    1 -
 248 files changed, 190 insertions(+), 516 deletions(-)

The rest:

 arch/arm/lib/uaccess_with_memcpy.c         |    1 +
 arch/x86/configs/x86_64_defconfig          |   29 +-
 arch/x86/kernel/microcode_core.c           |    6 +-
 block/bsg.c                                |    2 -
 block/compat_ioctl.c                       |    8 +-
 block/ioctl.c                              |   24 +-
 drivers/block/DAC960.c                     |    4 +-
 drivers/block/aoe/aoechr.c                 |    6 +-
 drivers/block/cciss.c                      |    4 +-
 drivers/block/paride/pg.c                  |    4 +-
 drivers/block/paride/pt.c                  |   16 +-
 drivers/char/briq_panel.c                  |    1 +
 drivers/char/dsp56k.c                      |    1 +
 drivers/char/hpet.c                        |   96 +++--
 drivers/char/mwave/mwavedd.c               |    1 +
 drivers/char/nvram.c                       |    7 +-
 drivers/char/pcmcia/cm4000_cs.c            |    1 +
 drivers/char/pcmcia/cm4040_cs.c            |    1 +
 drivers/char/random.c                      |    2 +
 drivers/char/snsc.c                        |    1 +
 drivers/char/tlclk.c                       |    1 +
 drivers/char/toshiba.c                     |    1 +
 drivers/char/xilinx_hwicap/xilinx_hwicap.c |    1 +
 drivers/gpu/vga/vgaarb.c                   |    4 +-
 drivers/hid/hidraw.c                       |    1 +
 drivers/input/serio/serio_raw.c            |    1 +
 drivers/isdn/capi/capifs.c                 |   10 +-
 drivers/md/dm-ioctl.c                      |    2 +
 drivers/media/dvb/dvb-core/dvbdev.c        |    1 +
 drivers/misc/phantom.c                     |    1 +
 drivers/pci/proc.c                         |    4 +-
 drivers/pcmcia/pcmcia_ioctl.c              |   23 +-
 drivers/sbus/char/display7seg.c            |    1 +
 drivers/sbus/char/jsflash.c                |   20 +-
 drivers/scsi/aacraid/linit.c               |    1 +
 drivers/scsi/ch.c                          |    1 +
 drivers/scsi/sg.c                          |   22 +-
 drivers/usb/core/file.c                    |    1 +
 drivers/usb/core/inode.c                   |    5 +
 drivers/usb/gadget/inode.c                 |   12 +-
 drivers/usb/misc/usblcd.c                  |    1 +
 drivers/usb/mon/mon_bin.c                  |   14 +-
 drivers/watchdog/cpwd.c                    |    1 +
 fs/adfs/super.c                            |    8 +-
 fs/affs/super.c                            |   14 +-
 fs/afs/super.c                             |    5 +
 fs/autofs/root.c                           |   67 +++-
 fs/autofs4/root.c                          |   69 +++-
 fs/bad_inode.c                             |    4 +
 fs/bfs/inode.c                             |    7 +-
 fs/block_dev.c                             |   20 +-
 fs/cifs/cifsfs.c                           |    9 +-
 fs/coda/inode.c                            |    8 +-
 fs/compat_ioctl.c                          |   43 +--
 fs/ecryptfs/main.c                         |    4 +
 fs/exec.c                                  |    6 -
 fs/exofs/super.c                           |    1 -
 fs/ext2/inode.c                            |    5 +-
 fs/ext2/super.c                            |   58 ++-
 fs/ext3/super.c                            |   12 -
 fs/ext4/super.c                            |   11 -
 fs/fat/dir.c                               |   11 +-
 fs/fat/fat.h                               |    2 +-
 fs/fat/file.c                              |   33 ++-
 fs/fat/namei_msdos.c                       |    7 +-
 fs/fat/namei_vfat.c                        |    7 +-
 fs/freevxfs/vxfs_lookup.c                  |    1 +
 fs/freevxfs/vxfs_super.c                   |    7 +-
 fs/hfs/super.c                             |    6 +-
 fs/hfsplus/super.c                         |    5 -
 fs/hpfs/super.c                            |    8 +-
 fs/hppfs/hppfs.c                           |    2 +-
 fs/ioctl.c                                 |    2 +
 fs/isofs/dir.c                             |    1 +
 fs/isofs/inode.c                           |    8 +-
 fs/jffs2/super.c                           |   11 +-
 fs/jfs/super.c                             |   22 +-
 fs/locks.c                                 |  110 +++--
 fs/namespace.c                             |    2 -
 fs/ncpfs/inode.c                           |    8 +-
 fs/nfs/super.c                             |   24 +
 fs/nilfs2/ioctl.c                          |    1 -
 fs/nilfs2/super.c                          |   10 -
 fs/ntfs/super.c                            |   24 +-
 fs/ocfs2/stack_user.c                      |    1 +
 fs/ocfs2/super.c                           |    5 +
 fs/proc/base.c                             |   10 +-
 fs/proc/inode.c                            |    8 +-
 fs/qnx4/dir.c                              |    1 +
 fs/qnx4/inode.c                            |    8 +-
 fs/read_write.c                            |    8 +
 fs/reiserfs/super.c                        |    4 +
 fs/smbfs/inode.c                           |    5 +
 fs/squashfs/super.c                        |    6 +
 fs/super.c                                 |    3 -
 fs/udf/super.c                             |    8 +-
 fs/ufs/super.c                             |    5 +
 include/linux/auto_fs.h                    |    1 +
 include/linux/blkdev.h                     |    6 +
 include/linux/ext2_fs_sb.h                 |    6 +
 include/linux/fs.h                         |    2 +
 include/linux/sched.h                      |    2 +
 include/linux/smp_lock.h                   |   57 ++-
 init/main.c                                |    5 -
 kernel/cgroup.c                            |    4 -
 kernel/perf_event.c                        |    2 +
 kernel/ptrace.c                            |   10 -
 kernel/sched.c                             |   17 +
 kernel/trace/blktrace.c                    |   14 +-
 kernel/trace/trace.c                       |    8 -
 lib/Kconfig.debug                          |   17 +
 lib/Makefile                               |    4 +
 lib/kernel_lock.c                          |  104 +-----
 lib/kernel_lock_core.c                     |  307 +++++++++++++
 net/sunrpc/cache.c                         |   30 +-
 net/sunrpc/rpc_pipe.c                      |    9 +-
 sound/core/timer.c                         |    5 +-
 sound/sound_core.c                         |    1 +
 virt/kvm/kvm_main.c                        |    6 +
 120 files changed, 1156 insertions(+), 540 deletions(-)

Yeah, the comment gives this:

	/*
	 * This lock_kernel fixes a subtle race with suid exec
	 */

But there is no lock_kernel() in the exec path, may be I missed it... so this may be an old lock_kernel() that doesn't exist anymore, and the bkl in the ptrace path is not going to help in any way.

What remain to check are the possible unintended racy places that this bkl might protect.

I'm going to check a first pass and if it looks fine, I'll just submit to Oleg and Roland.

	config USB
	        tristate "Support for Host-side USB"
	        depends on USB_ARCH_HAS_HCD && BKL

Well, that's very interesting definition of "obscure" :)

Jiri Kosina
SUSE Labs, Novell Inc.

That's why I said /mostly/ obscure modules. There are soundcore, usb-core, drm, vfat and a few other very common ones, along with many obscure ones.

Arnd

FWIW, this is the full list of 148 modules that require the BKL in an x86 allmodconfig, which is probably the configuration with the largest code coverage.

Arnd

sound/soundcore.ko
sound/soc/snd-soc-core.ko
sound/oss/sound.ko
sound/oss/msnd_pinnacle.ko
sound/oss/msnd_classic.ko
sound/core/snd.ko
sound/core/snd-pcm.ko
sound/core/seq/snd-seq.ko
sound/core/oss/snd-pcm-oss.ko
net/x25/x25.ko
net/wanrouter/wanrouter.ko
net/sunrpc/sunrpc.ko
net/irda/irnet/irnet.ko
net/irda/irda.ko
net/ipx/ipx.ko
net/appletalk/appletalk.ko
fs/ufs/ufs.ko
fs/udf/udf.ko
fs/squashfs/squashfs.ko
fs/smbfs/smbfs.ko
fs/reiserfs/reiserfs.ko
fs/qnx4/qnx4.ko
fs/ocfs2/ocfs2_stack_user.ko
fs/ocfs2/ocfs2.ko
fs/nfsd/nfsd.ko
fs/nfs/nfs.ko
fs/ncpfs/ncpfs.ko
fs/lockd/lockd.ko
fs/jffs2/jffs2.ko
fs/isofs/isofs.ko
fs/hpfs/hpfs.ko
fs/hfsplus/hfsplus.ko
fs/freevxfs/freevxfs.ko
fs/fat/vfat.ko
fs/fat/msdos.ko
fs/fat/fat.ko
fs/ecryptfs/ecryptfs.ko
fs/coda/coda.ko
fs/autofs4/autofs4.ko
fs/autofs/autofs.ko
fs/afs/kafs.ko
fs/adfs/adfs.ko
drivers/usb/misc/usblcd.ko
drivers/usb/misc/sisusbvga/sisusbvga.ko
drivers/usb/misc/rio500.ko
drivers/usb/misc/iowarrior.ko
drivers/usb/misc/idmouse.ko
drivers/usb/host/uhci-hcd.ko
drivers/usb/gadget/gadgetfs.ko
drivers/usb/gadget/g_printer.ko
drivers/usb/core/usbcore.ko
drivers/usb/class/usblp.ko
drivers/telephony/ixj.ko
drivers/scsi/st.ko
drivers/scsi/scsi_tgt.ko
drivers/scsi/pmcraid.ko
drivers/scsi/osst.ko
drivers/scsi/osd/osd.ko
drivers/scsi/mpt2sas/mpt2sas.ko
drivers/scsi/megaraid/megaraid_sas.ko
drivers/scsi/megaraid/megaraid_mm.ko
drivers/scsi/megaraid.ko
drivers/scsi/gdth.ko
drivers/scsi/dpt_i2o.ko
drivers/scsi/ch.ko
drivers/scsi/aacraid/aacraid.ko
drivers/scsi/3w-xxxx.ko
drivers/scsi/3w-sas.ko
drivers/scsi/3w-9xxx.ko
drivers/rtc/rtc-m41t80.ko
drivers/pci/hotplug/cpqphp.ko
drivers/net/wireless/ray_cs.ko
drivers/net/wireless/airo.ko
drivers/net/wan/cosa.ko
drivers/net/ppp_generic.ko
drivers/mtd/ubi/ubi.ko
drivers/mtd/mtdchar.ko
drivers/misc/phantom.ko
drivers/message/i2o/i2o_config.ko
drivers/message/fusion/mptctl.ko
drivers/media/video/zoran/zr36067.ko
drivers/media/video/videodev.ko
drivers/media/video/usbvision/usbvision.ko
drivers/media/video/usbvideo/vicam.ko
drivers/media/video/tlg2300/poseidon.ko
drivers/media/video/stv680.ko
drivers/media/video/stradis.ko
drivers/media/video/stkwebcam.ko
drivers/media/video/se401.ko
drivers/media/video/s2255drv.ko
drivers/media/video/pwc/pwc.ko
drivers/media/video/dabusb.ko
drivers/media/video/cx88/cx8800.ko
drivers/media/video/cx88/cx88-blackbird.ko
drivers/media/video/cx23885/cx23885.ko
drivers/media/video/cpia.ko
drivers/media/video/bt8xx/bttv.ko
drivers/media/radio/si470x/radio-usb-si470x.ko
drivers/media/dvb/ttpci/dvb-ttpci.ko
drivers/media/dvb/firewire/firedtv.ko
drivers/media/dvb/dvb-core/dvb-core.ko
drivers/media/dvb/bt8xx/dst_ca.ko
drivers/isdn/mISDN/mISDN_core.ko
drivers/isdn/i4l/isdn.ko
drivers/isdn/hysdn/hysdn.ko
drivers/isdn/hardware/eicon/divas.ko
drivers/isdn/hardware/eicon/diva_mnt.ko
drivers/isdn/hardware/eicon/diva_idi.ko
drivers/isdn/divert/dss1_divert.ko
drivers/isdn/capi/capifs.ko
drivers/isdn/capi/capi.ko
drivers/input/serio/serio_raw.ko
drivers/input/misc/uinput.ko
drivers/infiniband/core/rdma_ucm.ko
drivers/infiniband/core/ib_uverbs.ko
drivers/infiniband/core/ib_umad.ko
drivers/infiniband/core/ib_ucm.ko
drivers/ieee1394/video1394.ko
drivers/ieee1394/raw1394.ko
drivers/ieee1394/dv1394.ko
drivers/ide/ide-tape.ko
drivers/hwmon/fschmd.ko
drivers/hid/usbhid/usbhid.ko
drivers/hid/hid.ko
drivers/gpu/drm/i830/i830.ko
drivers/gpu/drm/i810/i810.ko
drivers/gpu/drm/drm.ko
drivers/firewire/firewire-core.ko
drivers/char/toshiba.ko
drivers/char/tlclk.ko
drivers/char/stallion.ko
drivers/char/raw.ko
drivers/char/ppdev.ko
drivers/char/pcmcia/cm4040_cs.ko
drivers/char/pcmcia/cm4000_cs.ko
drivers/char/mwave/mwave.ko
drivers/char/lp.ko
drivers/char/istallion.ko
drivers/char/ipmi/ipmi_watchdog.ko
drivers/char/ipmi/ipmi_devintf.ko
drivers/char/ip2/ip2.ko
drivers/char/i8k.ko
drivers/char/dtlk.ko
drivers/char/applicom.ko
drivers/block/pktcdvd.ko
drivers/block/paride/pt.ko
drivers/block/paride/pg.ko
drivers/block/DAC960.ko

...

...

...

firewire-core and raw1394 do not actually require the BKL, they only miss to declare their files as not seekable. I will post patches which change these accordingly.

My guess is that there is nothing to seek in dv1394, video1394, and firedtv either.

Needless to say, there may be other character device file interfaces which cannot be seeked but don't admit it yet.

Stefan Richter
-=====-==-=- --== ==-==
http://arcgraph.de/sr/

The <linux/firewire-cdev.h> character device file ABI is based on
- ioctl() to initiate actions,
- read() to consume events,
- mmap() for isochronous I/O DMA buffers.
lseek(), pread(), pwrite() (or any kind of write() at all) on the other hand are not applicable to /dev/fw* device files.

Alas, whereas for example file_operations.write == NULL causes write() to be failed with an appropriate error, file_operations.llseek == NULL causes fs/read_write.c::default_llseek to be called on lseek() per default.

This looks like not doing any harm, but it grabs the Big Kernel Lock. We don't want that, and we should return an error on lseek() and friends. This is provided by fs/read_write.c::no_llseek which we get if we clear the FMODE_LSEEK (and FMODE_PREAD, FMODE_PWRITE) flag by means of nonseekable_open().

Side note: The firewire-cdev interface has always been free of any BKL usage apart from this oversight regarding default_llseek (and from involuntary BKL usage by open() in older kernels).

---

Somebody correct me if I got anything wrong in my patch description.

This patch is motivated by Arnd's
"bkl removal: make unlocked_ioctl mandatory"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/firewire/core-cdev.c;h=4464b9dc01a8c69258a1e8880e9a390f17420b6c;hp=4eeaed57e2197a0dd5f0cab7cffa4713eaf2ec96;hb=05e7753338045e9ee3950b2da032c5e5774efa90;hpb=03165e1d096afb4b1d9cfccdad66eed038121cec
"BKL removal: mark remaining users as 'depends on BKL'"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/firewire/Kconfig;h=ae6d45900230dfbdc32c099d6a026ecfd0a6f5c2;hp=a9371b36a9b9c3074f5b31d7bdacf1bda72a15dd;hb=abb83d8fe5f8dcc8fca09bd9117429f73e1417e0;hpb=33c014b118f45516113d4b6823e40ea6f834dc6a

 drivers/firewire/core-cdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/drivers/firewire/core-cdev.c
===================================================================
@@ -226,7 +226,7 @@ static int fw_device_op_open(struct inod
 	list_add_tail(&client->link, &device->client_list);
 	mutex_unlock(&device->client_list_mutex);
 
-	return 0;
+	return nonseekable_open(inode, file);
 }
 
 static void queue_event(struct client *client, struct event *event,

Stefan Richter
-=====-==-=- --== ==-==
http://arcgraph.de/sr/

The raw1394 character device file ABI is based on
- write() or ioctl() to initiate actions, with write being used exactly like _IOW ioctls,
- read() to consume events,
- mmap() for isochronous I/O DMA buffers.

The video1394 character device file ABI is based on
- ioctl() to initiate actions,
- mmap() for isochronous I/O DMA buffers.

The dv1394 character device file ABI is based on
- ioctl() to initiate actions,
- read() and write() for simple serial reception and transmission of DV streams with a copy_to/from_user,
- mmap() for isochronous I/O DMA buffers for I/O without user copy.

lseek(), pread(), pwrite() on the other hand are not applicable to /dev/raw1394, /dev/video1394/*, and /dev/dv1394/* device files.

Alas, file_operations.llseek == NULL causes lseek() to call fs/read_write.c::default_llseek per default. This looks like not doing any harm in either of the three drivers, but it grabs the Big Kernel Lock. We don't want that, and we should return an error on lseek() and friends. This is provided by fs/read_write.c::no_llseek which we get if we clear the FMODE_LSEEK (and FMODE_PREAD, FMODE_PWRITE) flag by means of nonseekable_open().

Side note: Apart from this oversight regarding default_llseek, the raw1394, video1394, and dv1394 interfaces have been converted away from BKL usage some time ago. Although all this is legacy code which should be left in peace until it is eventually removed (as it is superseded by firewire-core's <linux/firewire-cdev.h> ABI), this change seems still worth doing to further minimize the presence of BKL usage in the kernel.

---

Somebody correct me if I misunderstood anything of these ABIs.

This patch is motivated by Arnd's
"bkl removal: make unlocked_ioctl mandatory"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/ieee1394/raw1394.c;h=7c312331fc14facd8d3e2f8cf05daf090fd88e71;hp=8aa56ac07e2920d371a5af41eca7f09c8b752380;hb=05e7753338045e9ee3950b2da032c5e5774efa90;hpb=03165e1d096afb4b1d9cfccdad66eed038121cec
etc., and
"BKL removal: mark remaining users as 'depends on BKL'"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/ieee1394/Kconfig;h=0d4954c2615233ccdd8ed28ba2e7b36ed1c7941d;hp=e02096cf7d95bfa383bddb703bbfbff6b782e688;hb=abb83d8fe5f8dcc8fca09bd9117429f73e1417e0;hpb=33c014b118f45516113d4b6823e40ea6f834dc6a

 drivers/ieee1394/dv1394.c    |    2 +-
 drivers/ieee1394/raw1394.c   |    2 +-
 drivers/ieee1394/video1394.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Index: b/drivers/ieee1394/dv1394.c
===================================================================
@@ -1824,7 +1824,7 @@ static int dv1394_open(struct inode *ino
 	       "and will not be available in the new firewire driver stack. "
 	       "Try libraw1394 based programs instead.\n", current->comm);
 
-	return 0;
+	return nonseekable_open(inode, file);
 }
 
 

Index: b/drivers/ieee1394/raw1394.c
===================================================================
@@ -2834,7 +2834,7 @@ static int raw1394_open(struct inode *in
 
 	file->private_data = fi;
 
-	return 0;
+	return nonseekable_open(inode, file);
 }
 
 static int raw1394_release(struct inode *inode, struct file *file)

Index: b/drivers/ieee1394/video1394.c
===================================================================
@@ -1239,7 +1239,7 @@ static int video1394_open(struct inode *
 	ctx->current_ctx = NULL;
 	file->private_data = ctx;
 
-	return 0;
+	return nonseekable_open(inode, file);
 }
 
 static int video1394_release(struct inode *inode, struct file *file)

Stefan Richter
-=====-==-=- --== ==-==
http://arcgraph.de/sr/

In order to remove Big Kernel Lock usages from the DVB subsystem, we need to
- provide .llseek file operations that do not grab the BKL (unlike fs/read_write.c::default_llseek) or mark files as not seekable,
- provide .unlocked_ioctl file operations.

Add two dvb_generic_ file operations for file interfaces which are not seekable and, respectively, do not require the BKL in their ioctl handlers.

Use them in one driver of which I am sure of that these are applicable. (Affected code paths were not runtime-tested since I don't have a CAM.)

Notes:

- In order to maximize code reuse and minimize API churn, I pass a dummy NULL struct inode * through dvb_usercopy() to dvbdev->kernel_ioctl(). Very ugly; it may be better to convert everything from .ioctl to .unlocked_ioctl in one go and remove the inode pointer argument everywhere.

- I suspect that actually all dvb_generic_open() users really want nonseekable_open --- then we should simply change dvb_generic_open() instead of adding dvb_generic_nonseekable_open() --- but I haven't checked other users of dvb_generic_open whether they require .llssek mehods other than fs/read_write.c::no_llseek.

---

This patch is motivated by Arnd's
"bkl removal: make unlocked_ioctl mandatory"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/media/dvb/firewire/firedtv-ci.c;h=7ab89035c101240a860d6c72bf8541a0fdf3aed9;hp=853e04b7cb361d45257f80dcafdcf121cd340c63;hb=05e7753338045e9ee3950b2da032c5e5774efa90;hpb=03165e1d096afb4b1d9cfccdad66eed038121cec
"BKL removal: mark remaining users as 'depends on BKL'"
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=blobdiff;f=drivers/media/dvb/firewire/Kconfig;h=ba8938e26414c8c82c41677f87c1361ed8fcebd0;hp=4afa29256df110d3383b6b469762cefbb46436b6;hb=abb83d8fe5f8dcc8fca09bd9117429f73e1417e0;hpb=33c014b118f45516113d4b6823e40ea6f834dc6a

 drivers/media/dvb/dvb-core/dvbdev.c     |   20 ++++++++++++++++++++
 drivers/media/dvb/dvb-core/dvbdev.h     |   11 +++++++----
 drivers/media/dvb/firewire/firedtv-ci.c |    4 ++--
 3 files changed, 29 insertions(+), 6 deletions(-)

Index: b/drivers/media/dvb/dvb-core/dvbdev.c
===================================================================
@@ -135,6 +135,18 @@ int dvb_generic_open(struct inode *inode
 EXPORT_SYMBOL(dvb_generic_open);
 
 
+int dvb_generic_nonseekable_open(struct inode *inode, struct file *file)
+{
+	int retval = dvb_generic_open(inode, file);
+
+	if (retval == 0)
+		retval = nonseekable_open(inode, file);
+
+	return retval;
+}
+EXPORT_SYMBOL(dvb_generic_nonseekable_open);
+
+
 int dvb_generic_release(struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev = file->private_data;

@@ -170,6 +182,14 @@ int dvb_generic_ioctl(struct inode *inod
 EXPORT_SYMBOL(dvb_generic_ioctl);
 
 
+long dvb_generic_unlocked_ioctl(struct file *file,
+				unsigned int cmd, unsigned long arg)
+{
+	return dvb_generic_ioctl(NULL, file, cmd, arg);
+}
+EXPORT_SYMBOL(dvb_generic_unlocked_ioctl);
+
+
 static int dvbdev_get_free_id (struct dvb_adapter *adap, int type)
 {
 	u32 id = 0;

Index: b/drivers/media/dvb/dvb-core/dvbdev.h
===================================================================
@@ -136,10 +136,13 @@ extern int dvb_register_device (struct d
 
 extern void dvb_unregister_device (struct dvb_device *dvbdev);
 
-extern int dvb_generic_open (struct inode *inode, struct file *file);
-extern int dvb_generic_release (struct inode *inode, struct file *file);
-extern int dvb_generic_ioctl (struct inode *inode, struct file *file,
-			      unsigned int cmd, unsigned long arg);
+extern int dvb_generic_open(struct inode *inode, struct file *file);
+extern int dvb_generic_nonseekable_open(struct inode *inode, struct file *file);
+extern int dvb_generic_release(struct inode *inode, struct file *file);
+extern int dvb_generic_ioctl(struct inode *inode, struct file *file,
+			     unsigned int cmd, unsigned long arg);
+extern long dvb_generic_unlocked_ioctl(struct file *file,
+				       unsigned int cmd, unsigned long arg);
 
 /* we don't mess with video_usercopy() any more,
 we simply define out own dvb_usercopy(), which will hopefully become

Index: b/drivers/media/dvb/firewire/firedtv-ci.c
===================================================================
@@ -217,8 +217,8 @@ static unsigned int fdtv_ca_io_poll(stru
 
 static const struct file_operations fdtv_ca_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= dvb_generic_ioctl,
-	.open		= dvb_generic_open,
+	.unlocked_ioctl	= dvb_generic_unlocked_ioctl,
+	.open		= dvb_generic_nonseekable_open,
 	.release	= dvb_generic_release,
 	.poll		= fdtv_ca_io_poll,
 };

Stefan Richter
-=====-==-=- --== ==-==
http://arcgraph.de/sr/

In order to remove Big Kernel Lock usages from the DVB subsystem, we need to
- provide .llseek file operations that do not grab the BKL (unlike fs/read_write.c::default_llseek) or mark files as not seekable,
- provide .unlocked_ioctl file operations.

Add two dvb_generic_ file operations for file interfaces which are not seekable and, respectively, do not require the BKL in their ioctl handlers.

Use them in one driver of which I am sure of that these are applicable. (Affected code paths in firedtv-ci were not runtime-tested since I don't have a CAM, but the frontend ioctls were of course runtime-tested.)

Notes:

- The dvb-core internal dvb_usercopy() API is changed to match .unlocked_ioctl() prototypes.

- I suspect that all dvb_generic_open() users really want nonseekable_open --- then we should simply change dvb_generic_open() instead of adding dvb_generic_nonseekable_open() --- but I haven't checked other users of dvb_generic_open whether they require .llssek mehods other than fs/read_write.c::no_llseek. Applies to:
drivers/media/dvb/ttpci/av7110.c drivers/media/dvb/ttpci/av7110_av.c drivers/media/dvb/ttpci/av7110_ca.c drivers/media/dvb/dvb-core/dvb_net.c drivers/media/dvb/dvb-core/dvb_frontend.c drivers/media/dvb/dvb-core/dvb_ca_en50221.c

- To be done by those who know the code: Check all users of struct dvb_device.kernel_ioctl whether they really need the BKL. Convert to .unlocked_ioctl and remove .kernel_ioctl and the temporarily introduced dvbdev.c::legacy_usercopy(). Applies to:
drivers/media/dvb/ttpci/av7110.c drivers/media/dvb/ttpci/av7110_av.c drivers/media/dvb/ttpci/av7110_ca.c drivers/media/dvb/dvb-core/dvb_frontend.c

---

Update: Split dvb_usercopy into one that follows the .unlocked_ioctl prototype form and another one that preserves the old form.

 drivers/media/dvb/dvb-core/dmxdev.c         |   10 -
 drivers/media/dvb/dvb-core/dvb_ca_en50221.c |    6 
 drivers/media/dvb/dvb-core/dvb_net.c        |    5 
 drivers/media/dvb/dvb-core/dvbdev.c         |  190 +++++++++++++-------
 drivers/media/dvb/dvb-core/dvbdev.h         |   23 +-
 drivers/media/dvb/firewire/firedtv-ci.c     |    9 
 6 files changed, 148 insertions(+), 95 deletions(-)

Index: b/drivers/media/dvb/dvb-core/dmxdev.c
===================================================================
@@ -963,8 +963,7 @@ dvb_demux_read(struct file *file, char _
 	return ret;
 }
 
-static int dvb_demux_do_ioctl(struct inode *inode, struct file *file,
-			      unsigned int cmd, void *parg)
+static long dvb_demux_do_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
 	struct dmxdev_filter *dmxdevfilter = file->private_data;
 	struct dmxdev *dmxdev = dmxdevfilter->dev;

@@ -1087,7 +1086,7 @@ static int dvb_demux_do_ioctl(struct ino
 static int dvb_demux_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_demux_do_ioctl);
+	return dvb_usercopy(file, cmd, arg, dvb_demux_do_ioctl);
 }
 
 static unsigned int dvb_demux_poll(struct file *file, poll_table *wait)

@@ -1152,8 +1151,7 @@ static struct dvb_device dvbdev_demux = 
 	.fops = &dvb_demux_fops
 };
 
-static int dvb_dvr_do_ioctl(struct inode *inode, struct file *file,
-			    unsigned int cmd, void *parg)
+static long dvb_dvr_do_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dmxdev *dmxdev = dvbdev->priv;

@@ -1179,7 +1177,7 @@ static int dvb_dvr_do_ioctl(struct inode
 static int dvb_dvr_ioctl(struct inode *inode, struct file *file,
 			 unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_dvr_do_ioctl);
+	return dvb_usercopy(file, cmd, arg, dvb_dvr_do_ioctl);
 }
 
 static unsigned int dvb_dvr_poll(struct file *file, poll_table *wait)

Index: b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
===================================================================
@@ -1181,8 +1181,8 @@ static int dvb_ca_en50221_thread(void *d
  *
  * @return 0 on success, <0 on error.
  */
-static int dvb_ca_en50221_io_do_ioctl(struct inode *inode, struct file *file,
-				      unsigned int cmd, void *parg)
+static long dvb_ca_en50221_io_do_ioctl(struct file *file,
+				       unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_ca_private *ca = dvbdev->priv;

@@ -1258,7 +1258,7 @@ static int dvb_ca_en50221_io_do_ioctl(st
 static int dvb_ca_en50221_io_ioctl(struct inode *inode, struct file *file,
 				   unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_ca_en50221_io_do_ioctl);
+	return dvb_usercopy(file, cmd, arg, dvb_ca_en50221_io_do_ioctl);
 }
 
 

Index: b/drivers/media/dvb/dvb-core/dvb_net.c
===================================================================
@@ -1333,8 +1333,7 @@ static int dvb_net_remove_if(struct dvb_
 	return 0;
 }
 
-static int dvb_net_do_ioctl(struct inode *inode, struct file *file,
-		  unsigned int cmd, void *parg)
+static long dvb_net_do_ioctl(struct file *file, unsigned int cmd, void *parg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_net *dvbnet = dvbdev->priv;

@@ -1438,7 +1437,7 @@ static int dvb_net_do_ioctl(struct inode
 static int dvb_net_ioctl(struct inode *inode, struct file *file,
 	      unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_net_do_ioctl);
+	return dvb_usercopy(file, cmd, arg, dvb_net_do_ioctl);
 }
 
 static int dvb_net_close(struct inode *inode, struct file *file)

Index: b/drivers/media/dvb/dvb-core/dvbdev.h
===================================================================
@@ -118,6 +118,7 @@ struct dvb_device {
 	/* don't really need those !? -- FIXME: use video_usercopy  */
 	int (*kernel_ioctl)(struct inode *inode, struct file *file,
 			    unsigned int cmd, void *arg);
+	long (*unlocked_ioctl)(struct file *file, unsigned int cmd, void *arg);
 
 	void *priv;
 };

@@ -136,19 +137,15 @@ extern int dvb_register_device (struct d
 
 extern void dvb_unregister_device (struct dvb_device *dvbdev);
 
-extern int dvb_generic_open (struct inode *inode, struct file *file);
-extern int dvb_generic_release (struct inode *inode, struct file *file);
-extern int dvb_generic_ioctl (struct inode *inode, struct file *file,
-			      unsigned int cmd, unsigned long arg);
-
-/* we don't mess with video_usercopy() any more,
-we simply define out own dvb_usercopy(), which will hopefully become
-generic_usercopy()  someday... */
-
-extern int dvb_usercopy(struct inode *inode, struct file *file,
-			    unsigned int cmd, unsigned long arg,
-			    int (*func)(struct inode *inode, struct file *file,
-			    unsigned int cmd, void *arg));
+extern int dvb_generic_open(struct inode *inode, struct file *file);
+extern int dvb_generic_nonseekable_open(struct inode *inode, struct file *file);
+extern int dvb_generic_release(struct inode *inode, struct file *file);
+extern int dvb_generic_ioctl(struct inode *inode, struct file *file,
+			     unsigned int cmd, unsigned long arg);
+extern long dvb_generic_unlocked_ioctl(struct file *file,
+					unsigned int cmd, unsigned long arg);
+extern long dvb_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
+		long (*func)(struct file *file, unsigned int cmd, void *arg));
 
 /** generic DVB attach function. */
 #ifdef CONFIG_MEDIA_ATTACH

Index: b/drivers/media/dvb/firewire/firedtv-ci.c
===================================================================
@@ -175,8 +175,7 @@ static int fdtv_ca_send_msg(struct fired
 	return err;
 }
 
-static int fdtv_ca_ioctl(struct inode *inode, struct file *file,
-			    unsigned int cmd, void *arg)
+static long fdtv_ca_ioctl(struct file *file, unsigned int cmd, void *arg)
 {
 	struct dvb_device *dvbdev = file->private_data;
 	struct firedtv *fdtv = dvbdev->priv;

@@ -217,8 +216,8 @@ static unsigned int fdtv_ca_io_poll(stru
 
 static const struct file_operations fdtv_ca_fops = {
 	.owner		= THIS_MODULE,
-	.ioctl		= dvb_generic_ioctl,
-	.open		= dvb_generic_open,
+	.unlocked_ioctl	= dvb_generic_unlocked_ioctl,
+	.open		= dvb_generic_nonseekable_open,
 	.release	= dvb_generic_release,
 	.poll		= fdtv_ca_io_poll,
 };

@@ -228,7 +227,7 @@ static struct dvb_device fdtv_ca = {
 	.readers	= 1,
 	.writers	= 1,
 	.fops		= &fdtv_ca_fops,
-	.kernel_ioctl	= fdtv_ca_ioctl,
+	.unlocked_ioctl	= fdtv_ca_ioctl,
 };
 
 int fdtv_ca_register(struct firedtv *fdtv)

Stefan Richter
-=====-==-=- --== ===--
http://arcgraph.de/sr/

Your patches look good, but it would be helpful to also set .llseek = no_llseek in the file operations, because that is much easier to grep for than only the nonseekable_open. While it's technically a NOP on the presence of nonseekable_open, it will help that I don't accidentally apply my patch on top of yours.

Arnd

Sounds like a plan, but (a) if my .llseek = no_llseek and your .llseek = default_llseek are not within diff context range, you (or whoever else merges mine and yours) only get a compiler warning (Initializer entry defined twice) rather than a merge conflict which couldn't be missed,
(b) there won't be a merge conflict in "BKL removal: mark remaining users as 'depends on BKL'". (c) While I don't mind adding more visual clutter to ieee1394/*, I prefer terse coding in firewire/*.

How about I put my nonseekable_open additions into a release branch and send you a pull request after a few days exposure in linux-next? If you do not plan to respin your patch queue soon or at all, I could even let you pull a for-arnd branch with a semantically correct merge of yours and mine.

General thoughts:

".llseek = NULL," so far meant "do the Right Thing on lseek() and friends, as far as the fs core can tell". Shouldn't we keep it that way? It's as close to other ".method = NULL," as it can get, which either mean "silently skip this method if it doesn't matter" (e.g. .flush) or "fail attempts to use this method with a fitting errno" (e.g. .write).

Of course, as we have already seen with infiniband, firewire, ieee1394, .llseek = NULL is ambiguous in practice. Does the driver really want to use default_llseek, or should it rather use no_llseek and/or nonseekable_open, or should it even implement a dummy_llseek() { return 0; } which avoids the BKL but preserves ABI behaviour? This needs to be resolved for each and every case eventually, regardless of whether or when your addition of .llseek = default_llseek enters mainline.

Stefan Richter
-=====-==-=- --== ===--
http://arcgraph.de/sr/

I can probably remember this specific one now, but for other people doing the same on their subsystems, adding no_llseek may help reduce the need for coordination.

My series changes the default from 'default_llseek' to 'generic_file_llseek', which is almost identical, except for taking the inode mutex instead of the BKL. Another option that has been discussed before is to make no_llseek the default, but that might cause more serious problems wiht drivers that really require seeking.

Since using default_llseek can only ever make a difference if the driver actually uses the BKL in any other function, I could go through the patches again and revert those that do no use the BKL anywhere else.

Yes, that also sounds like a good idea. I believe that Jan actually posted a patch to do that at some point.

Arnd

What if another file operation changes the file pointer while holding the bkl? You're not protected anymore in this case.

Exactly, that's why I changed all the drivers to set default_llseek explicitly. Even this is very likely not needed in more than a handful of drivers (if any), for a number of reasons:

- sys_read/sys_write *never* hold any locks while calling file_pos_write(), which is the only place they get updated for regular files.
- concurrent llseek plus other file operations on the same file descriptor usually already have an undefined outcome.
- when I started inspecting drivers that look at file->f_pos themselves (not the read/write operation arguments), I found that practically all of them are doing this in a totally broken way!
- The only think we'd probably ever want to lock against in llseek is readdir, which is not used in any drivers, but only in file systems.

Arnd

Ah ok.

Yeah sure. But the pushdown (or step by step replacement with generic_file_llseek) is still necessary to ensure every places are fine.

Yeah.

Hehe :)

Right.

That is not that easy. generic_file_llseek() is testing against 'offset < inode->i_sb->s_maxbytes'. This is not necessarily true when you think about directories with random offset cookies. I know that seeking on directories is stupid but don't blame me.

Yes, it is in

http://git.infradead.org/users/jblunck/linux-2.6.git bkl/default-lseek

There are some other patches in that branch that are not upstream yet. Mind to take them for your bkl-removal branch?

Cheers,
Jan

Oh, I see. Would it work if we extend generic_file_llseek to only check s_maxbytes if S_ISREG(inode->i_mode)))?

Frederic is now collecting the new patches. Your default-lseek series looks good to me, except for the obvious one that says 'FIXME' in the subject.

Maybe Frederic can add your series except for that one as another branch to get pulled into his kill-the-bkl master branch.

Arnd

Ok, will have a look at this soon (I will also put a branch for the procfs series).

Thanks.

Yes and maybe rename generic_file_llseek to generic_llseek.

Jan

I pushed modified versions of these patches out to linux1394-2.6.git now (master and for-next branch, on top of unrelated firewire updates). They contain the explicit .llseek = no_llseek now.

http://git.kernel.org/?p=linux/kernel/git/ieee1394/linux1394-2.6.git;a=commitdiff;h=3ac26b2ee30005930117fe6a180c139c5f300faf
http://git.kernel.org/?p=linux/kernel/git/ieee1394/linux1394-2.6.git;a=commitdiff;h=7cfe21aae155c26193fde617dc61d37a79a63f86

Stefan Richter
-=====-==-=- -=-- -=-=-
http://arcgraph.de/sr/

Except for those two guys in core/*.c:

                        /* keep API that guarantees BKL */
                        lock_kernel();
                        retval = driver->ioctl(intf, ctl->ioctl_code, buf);
                        unlock_kernel();
                        if (retval == -ENOIOCTLCMD)
                                retval = -ENOTTY;

I guess that could be just moved into the low level modules with unlocked_ioctl

And one use in the usbfs which seems quite bogus and can be probably removed.

-Andi

[email protected] -- Speaking for myself only.

As a followup:

I killed some time by going through the various BKL uses in USB and came up with this git tree to address them . Feel free to integrate into your tree.

With this only some obscure USB low level drivers still need to depend on BKL, the majority is clean. Gadgetfs also still needs it for now.

I ended up also fixing some minor races in usb serial registration/ unregistration.

Opens:
- The usb serial ioctl entry needs to become a unlocked_ioctl, but I think that needs your tree first. The code below it doesn't need it anymore.
- The seek function in uhci-debug.c probably is still racy.

Only lightly tested. Some more reviewing would be appreciated

-Andi

The following changes since commit b72c40949b0f04728f2993a1434598d3bad094ea:
Linus Torvalds (1):

        Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/pci-2.6

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc-2.6.git usb-bkl

Andi Kleen (12):
USB-BKL: Remove lock_kernel in usbfs update_sb()
USB-BKL: Remove BKL from usb serial drivers ioctl handlers
USB-BKL: Convert usb_driver ioctl to unlocked_ioctl
USB-BKL: Remove BKL use for usb serial driver probing
USB-BKL: Make usb monitor depend on BKL
USB-BKL: Make usb lcd driver depend on BKL
USB-BKL: Make usb sisvga driver depend on BKL
USB-BKL: Make usb rio500 driver depend on BKL
USB-BKL: Make usb iowarrior driver depend on BKL
USB-BKL: Remove BKL use in uhci-debug
USB-BKL: Make usb gadget fs depend on BKL
USB-BKL: Make usb gadget printer depend on BKL

 drivers/usb/core/devio.c              |    7 +----
 drivers/usb/core/hub.c                |    3 +-
 drivers/usb/core/inode.c              |    4 ---
 drivers/usb/gadget/Kconfig            |    3 +-
 drivers/usb/host/uhci-debug.c         |   17 +++++----------
 drivers/usb/misc/Kconfig              |    6 ++--
 drivers/usb/misc/sisusbvga/Kconfig    |    2 +-
 drivers/usb/misc/usbtest.c            |    3 +-
 drivers/usb/mon/Kconfig               |    2 +-
 drivers/usb/serial/ark3116.c          |    3 +-
 drivers/usb/serial/ch341.c            |    3 +-
 drivers/usb/serial/cypress_m8.c       |    8 +++---
 drivers/usb/serial/ftdi_sio.c         |    4 +-
 drivers/usb/serial/io_tables.h        |    8 +++---
 drivers/usb/serial/io_ti.c            |    5 ++-
 drivers/usb/serial/kobil_sct.c        |    3 +-
 drivers/usb/serial/mos7720.c          |    3 +-
 drivers/usb/serial/mos7840.c          |    3 +-
 drivers/usb/serial/opticon.c          |    3 +-
 drivers/usb/serial/oti6858.c          |    3 +-
 drivers/usb/serial/pl2303.c           |    3 +-
 drivers/usb/serial/spcp8x5.c          |    2 +-
 drivers/usb/serial/ti_usb_3410_5052.c |    6 ++--
 drivers/usb/serial/usb-serial.c       |   36 ++++++++++++++++----------------
 drivers/usb/serial/whiteheat.c        |    4 +-
 include/linux/usb.h                   |    2 +-
 include/linux/usb/serial.h            |    2 +-
 27 files changed, 74 insertions(+), 74 deletions(-)

[email protected] -- Speaking for myself only.

Right.

Great, I was hoping someone picks up the missing pieces. Maybe someone else can do the same for the sound drivers ;-).

Sure, that absolutely makes sense.

The usb-serial driver has a few instances where it takes the BKL in the mainline code, but this gets converted to the Big TTY Mutex in my series. The ioctl method in there is fine as far as I can tell.

That function could be removed in favor of using generic_file_ioctl and setting i_size to up->size.

Also, the race is only between concurrent calls of llseek on the same file descriptor, which is undefined anyway. The current code also doesn't protect you against partial updates of f_pos during ->read() on 32 bit systems (nothing ever does), and it even fails to protect against the concurrent llseek race because the assignment is done outside of the f_pos update.

The serial_ioctl function is already called without the BKL, depite the name. tty_operations->ioctl was converted a long time ago, so I guess this patch can be dropped from your series.

Arnd

Does that lock against read in libfs?

That is not what I meant.

I wasn't sure it would protect against parallel reads.

Does it?

I think a rename is better, I take compile errors over subtle breakage any day.

Ok.

-Andi

[email protected] -- Speaking for myself only.

No.

There is no way for any driver or file system right now to protect against that, nor has there been for a long time[1]. The sys_read and sys_write functions use file_pos_write() to update the file->f_pos without taking any lock, and they pass a local variable into the *ppos argument of the ->read/->write file operations, which means that the file operation itself cannot add locking to the update either.

We never do in-place updates of file->f_pos, but on architectures where a 64 bit load can see incorrect data from a 64 bit store, any concurrent read/write/llseek combinations may cause problems, except for two concurrent lseek. Also, llseek is usually serialized with readdir/getdents for file systems.

ok, fine with me.

Arnd

[1] http://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commitdiff;h=55f09ec0087c160533eab791607d92c9ce6222ae
was merged in linux-2.6.8, which opened this race.

I updated the git tree removing the unneeded serial change. Feel free to pull.

git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc-2.6.git usb-bkl

-Andi

[email protected] -- Speaking for myself only.

Looks good to me. I guess it makes sense to merge this through Greg's USB tree. AFAICT the only prerequisite to this series is to have CONFIG_BKL enabled in Kconfig. Shall we add a that to 2.6.34 as 'def_bool y' for preparation so we can queue up patches like this in maintainer trees?

Arnd

There is a typo in this one:

commit 12c8fcce56c0de4fdcacf048fe723c8778af940d

Author: Arnd Bergmann <[email protected]>
Date:   Wed Mar 24 20:08:55 2010 +0100

block: replace BKL with global mutex

It doesn't seem to interact with much else, so give this a try.

index d9d6206..9c1277a 100644

[ snip ]

@@ -1641,7 +1643,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,

        ret = -ENXIO;

- lock_kernel();

+       mutex_unlock(&blkdev_mutex);
        p = dev_to_part(dev);
        bdev = bdget(part_devt(p));
        if (bdev == NULL)

I just queued the perf_event one. It looks pretty good. I'm also looking at some of the most trivials (ehm..less hards) in the list and see which we can submit right away.

Thanks.

This one is upstream now.

About this one, there is a "sensible" part:

@@ -1943,7 +1949,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,
+ .llseek = generic_file_llseek,
.read = proc_fdinfo_read,
};

Replacing default_llseek() by generic_file_llseek() as you did for most of the other parts is fine.

But the above changes the semantics as it makes it seekable. Why not just keeping it as is? It just ends up in no_llseek().

There is also the ioctl part that takes the bkl in procfs. I'll just check nothing weird happens there wrt file pos. We probably first need to pushdown the bkl in the procfs ioctl handlers.

The default is default_llseek, which uses the BKL and cannot be used if procfs is builtin and the BKL is a module.

The BKL in procfs is only for proc files that have registered their own .ioctl instead of .unlocked_ioctl method. Converting every file_operations instance to provide an unlocked_ioctl (as one of the other patches does) makes sure that this path is never taken. BTW, there are less than a handful of procfs files that provide an ioctl operation, and those probably should never have been merged.

Arnd

Yeah, but you removed the nonseekable_open and made generic_file_llseek in llseek on this one.
This makes it seekable while it wasn't, changing its ABI.
It wasn't taking the bkl before that as it was calling no_llseek().

May be its non seekable property is irrelevant, I don't know, but if this behaviour must be changed, it should be in a separate patch as that dosn't deal with the bkl.

There are three of them. I'm going to make them .unlocked_ioctl and push the bkl inside, and warn on further uses of .ioctl, without applying the bkl there anymore.

That plus your bkl removal in proc seek, should totally remove the bkl from procfs.

Ah, I see what you mean. That change was certainly not intentional an should be reverted. Thanks for pointing this out.

Ok

Arnd

Great, Arnd, I like this.

I also have a private but stale tree where I have collected some remove bkl patches (which I will review against your tree.) I think that it is important that we keep chipping away at it though, and that we all keep sending stuff upstream when it is ready.

Thanks
John

Hi Arnd,

Looking at your tree, I see you have commit 753dd249 ("perf_event: use nonseekable_open") that does:

Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Good point, I'll update that.

Thanks.

You're right. I did not consider this.

Arnd