[PATCH 0/5] Kill the BKL in compat ioctl handling

[PATCH 0/5] Kill the BKL in compat ioctl handling

[Arnd Bergmann]

I noticed that compat_sys_ioctl still holds the big kernel
lock for all ioctl conversion handlers. It would be easy to
push this one level down and only do it where necessary,
but I decided to clean it up some more at the same time
and move all the VT ioctl handling into the specific drivers.

	Arnd <><

  arch/um: handle compat_ioctl in tty line driver
  s390: move keyboard compat ioctls into tty3270 driver
  tty: handle VT specific compat ioctls in vt driver
  compat_ioctl: remove VT specific ioctl handlers
  compat_ioctl: do not hold BKL in handlers

 arch/um/drivers/line.c          |    6 +
 arch/um/drivers/ssl.c           |    1 +
 arch/um/drivers/stdio_console.c |    1 +
 arch/um/include/shared/line.h   |    2 +
 drivers/char/vt.c               |    3 +
 drivers/char/vt_ioctl.c         |  203 +++++++++++++++++++++++++++++++++++++
 drivers/s390/char/tty3270.c     |   19 ++++
 fs/compat_ioctl.c               |  213 ---------------------------------------
 include/linux/tty.h             |    3 +
 9 files changed, 238 insertions(+), 213 deletions(-)

[PATCH 1/5] arch/um: handle compat_ioctl in tty line driver

[Arnd Bergmann]

This pushes the handling of VT specific ioctls down to the
UML specific drivers so we can remove it from the common
compat_ioctl.c file.

Cc: Jeff Dike <jdike@addtoit.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/um/drivers/line.c          |    6 ++++++
 arch/um/drivers/ssl.c           |    1 +
 arch/um/drivers/stdio_console.c |    1 +
 arch/um/include/shared/line.h   |    2 ++
 4 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index 14a102e..9819a3b 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -314,6 +314,12 @@ int line_ioctl(struct tty_struct *tty, struct file * file,
 	return ret;
 }
 
+long line_compat_ioctl(struct tty_struct *tty, struct file * file,
+	       unsigned int cmd, unsigned long arg)
+{
+	return line_ioctl(tty, file, cmd, arg);
+}
+
 void line_throttle(struct tty_struct *tty)
 {
 	struct line *line = tty->driver_data;
diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
index f1786e6..eccf14a 100644
--- a/arch/um/drivers/ssl.c
+++ b/arch/um/drivers/ssl.c
@@ -137,6 +137,7 @@ static const struct tty_operations ssl_ops = {
 	.flush_chars 		= line_flush_chars,
 	.set_termios 		= line_set_termios,
 	.ioctl 	 		= line_ioctl,
+	.compat_ioctl 	 	= line_compat_ioctl,
 	.throttle 		= line_throttle,
 	.unthrottle 		= line_unthrottle,
 #if 0
diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
index 49266f6..2cb1f04 100644
--- a/arch/um/drivers/stdio_console.c
+++ b/arch/um/drivers/stdio_console.c
@@ -120,6 +120,7 @@ static const struct tty_operations console_ops = {
 	.flush_chars 		= line_flush_chars,
 	.set_termios 		= line_set_termios,
 	.ioctl 	 		= line_ioctl,
+	.compat_ioctl 	 	= line_compat_ioctl,
 	.throttle 		= line_throttle,
 	.unthrottle 		= line_unthrottle,
 };
diff --git a/arch/um/include/shared/line.h b/arch/um/include/shared/line.h
index 311a0d3..6ef62e4 100644
--- a/arch/um/include/shared/line.h
+++ b/arch/um/include/shared/line.h
@@ -79,6 +79,8 @@ extern void line_flush_chars(struct tty_struct *tty);
 extern int line_write_room(struct tty_struct *tty);
 extern int line_ioctl(struct tty_struct *tty, struct file * file,
 		      unsigned int cmd, unsigned long arg);
+extern long line_compat_ioctl(struct tty_struct *tty, struct file * file,
+		      unsigned int cmd, unsigned long arg);
 extern void line_throttle(struct tty_struct *tty);
 extern void line_unthrottle(struct tty_struct *tty);
 
-- 
1.6.3.3

Re: [PATCH 1/5] arch/um: handle compat_ioctl in tty line driver

[Amerigo Wang]

On Thu, Aug 06, 2009 at 03:09:26PM +0200, Arnd Bergmann wrote:
>This pushes the handling of VT specific ioctls down to the
>UML specific drivers so we can remove it from the common
>compat_ioctl.c file.
>
>Cc: Jeff Dike <jdike@addtoit.com>
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Acked-by: WANG Cong <xiyou.wangcong@gmail.com>



>---
> arch/um/drivers/line.c          |    6 ++++++
> arch/um/drivers/ssl.c           |    1 +
> arch/um/drivers/stdio_console.c |    1 +
> arch/um/include/shared/line.h   |    2 ++
> 4 files changed, 10 insertions(+), 0 deletions(-)
>
>diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
>index 14a102e..9819a3b 100644
>--- a/arch/um/drivers/line.c
>+++ b/arch/um/drivers/line.c
>@@ -314,6 +314,12 @@ int line_ioctl(struct tty_struct *tty, struct file * file,
> 	return ret;
> }
> 
>+long line_compat_ioctl(struct tty_struct *tty, struct file * file,
>+	       unsigned int cmd, unsigned long arg)
>+{
>+	return line_ioctl(tty, file, cmd, arg);
>+}
>+
> void line_throttle(struct tty_struct *tty)
> {
> 	struct line *line = tty->driver_data;
>diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
>index f1786e6..eccf14a 100644
>--- a/arch/um/drivers/ssl.c
>+++ b/arch/um/drivers/ssl.c
>@@ -137,6 +137,7 @@ static const struct tty_operations ssl_ops = {
> 	.flush_chars 		= line_flush_chars,
> 	.set_termios 		= line_set_termios,
> 	.ioctl 	 		= line_ioctl,
>+	.compat_ioctl 	 	= line_compat_ioctl,
> 	.throttle 		= line_throttle,
> 	.unthrottle 		= line_unthrottle,
> #if 0
>diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
>index 49266f6..2cb1f04 100644
>--- a/arch/um/drivers/stdio_console.c
>+++ b/arch/um/drivers/stdio_console.c
>@@ -120,6 +120,7 @@ static const struct tty_operations console_ops = {
> 	.flush_chars 		= line_flush_chars,
> 	.set_termios 		= line_set_termios,
> 	.ioctl 	 		= line_ioctl,
>+	.compat_ioctl 	 	= line_compat_ioctl,
> 	.throttle 		= line_throttle,
> 	.unthrottle 		= line_unthrottle,
> };
>diff --git a/arch/um/include/shared/line.h b/arch/um/include/shared/line.h
>index 311a0d3..6ef62e4 100644
>--- a/arch/um/include/shared/line.h
>+++ b/arch/um/include/shared/line.h
>@@ -79,6 +79,8 @@ extern void line_flush_chars(struct tty_struct *tty);
> extern int line_write_room(struct tty_struct *tty);
> extern int line_ioctl(struct tty_struct *tty, struct file * file,
> 		      unsigned int cmd, unsigned long arg);
>+extern long line_compat_ioctl(struct tty_struct *tty, struct file * file,
>+		      unsigned int cmd, unsigned long arg);
> extern void line_throttle(struct tty_struct *tty);
> extern void line_unthrottle(struct tty_struct *tty);
> 
>-- 
>1.6.3.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/5] s390: move keyboard compat ioctls into tty3270 driver

[Arnd Bergmann]

All keyboard ioctls are compatible, so we can simply
move the compat handling into the vt and tty3270 drivers.

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/s390/char/tty3270.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 3838567..86b2889 100644
--- a/drivers/s390/char/tty3270.c
+++ b/drivers/s390/char/tty3270.c
@@ -1731,6 +1731,22 @@ tty3270_ioctl(struct tty_struct *tty, struct file *file,
 	return kbd_ioctl(tp->kbd, file, cmd, arg);
 }
 
+#ifdef CONFIG_COMPAT
+static long
+tty3270_compat_ioctl(struct tty_struct *tty, struct file *file,
+	      unsigned int cmd, unsigned long arg)
+{
+	struct tty3270 *tp;
+
+	tp = tty->driver_data;
+	if (!tp)
+		return -ENODEV;
+	if (tty->flags & (1 << TTY_IO_ERROR))
+		return -EIO;
+	return kbd_ioctl(tp->kbd, file, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
 static const struct tty_operations tty3270_ops = {
 	.open = tty3270_open,
 	.close = tty3270_close,
@@ -1745,6 +1761,9 @@ static const struct tty_operations tty3270_ops = {
 	.hangup = tty3270_hangup,
 	.wait_until_sent = tty3270_wait_until_sent,
 	.ioctl = tty3270_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = tty3270_compat_ioctl,
+#endif
 	.set_termios = tty3270_set_termios
 };
 
-- 
1.6.3.3

[PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Arnd Bergmann]

The VT specific compat_ioctl handlers are the only ones
in common code that require the BKL. Moving them into
the vt driver lets us remove the BKL from the other handlers
and cleans up the code.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/vt.c       |    3 +
 drivers/char/vt_ioctl.c |  203 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/tty.h     |    3 +
 3 files changed, 209 insertions(+), 0 deletions(-)

diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 404f4c1..56c2606 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -2910,6 +2910,9 @@ static const struct tty_operations con_ops = {
 	.flush_chars = con_flush_chars,
 	.chars_in_buffer = con_chars_in_buffer,
 	.ioctl = vt_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = vt_compat_ioctl,
+#endif
 	.stop = con_stop,
 	.start = con_start,
 	.throttle = con_throttle,
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 95189f2..3389bfe 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -16,6 +16,7 @@
 #include <linux/tty.h>
 #include <linux/timer.h>
 #include <linux/kernel.h>
+#include <linux/compat.h>
 #include <linux/kd.h>
 #include <linux/vt.h>
 #include <linux/string.h>
@@ -1170,6 +1171,208 @@ eperm:
 	goto out;
 }
 
+#ifdef CONFIG_COMPAT
+
+struct compat_consolefontdesc {
+	unsigned short charcount;       /* characters in font (256 or 512) */
+	unsigned short charheight;      /* scan lines per character (1-32) */
+	compat_caddr_t chardata;	/* font data in expanded form */
+};
+
+static inline int
+compat_fontx_ioctl(int cmd, struct compat_consolefontdesc __user *user_cfd,
+			 int perm, struct console_font_op *op)
+{
+	struct compat_consolefontdesc cfdarg;
+	int i;
+
+	if (copy_from_user(&cfdarg, user_cfd, sizeof(struct compat_consolefontdesc)))
+		return -EFAULT;
+
+	switch (cmd) {
+	case PIO_FONTX:
+		if (!perm)
+			return -EPERM;
+		op->op = KD_FONT_OP_SET;
+		op->flags = KD_FONT_FLAG_OLD;
+		op->width = 8;
+		op->height = cfdarg.charheight;
+		op->charcount = cfdarg.charcount;
+		op->data = compat_ptr(cfdarg.chardata);
+		return con_font_op(vc_cons[fg_console].d, op);
+	case GIO_FONTX:
+		op->op = KD_FONT_OP_GET;
+		op->flags = KD_FONT_FLAG_OLD;
+		op->width = 8;
+		op->height = cfdarg.charheight;
+		op->charcount = cfdarg.charcount;
+		op->data = compat_ptr(cfdarg.chardata);
+		i = con_font_op(vc_cons[fg_console].d, op);
+		if (i)
+			return i;
+		cfdarg.charheight = op->height;
+		cfdarg.charcount = op->charcount;
+		if (copy_to_user(user_cfd, &cfdarg, sizeof(struct compat_consolefontdesc)))
+			return -EFAULT;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+struct compat_console_font_op {
+	compat_uint_t op;        /* operation code KD_FONT_OP_* */
+	compat_uint_t flags;     /* KD_FONT_FLAG_* */
+	compat_uint_t width, height;     /* font size */
+	compat_uint_t charcount;
+	compat_caddr_t data;    /* font data with height fixed to 32 */
+};
+
+static inline int
+compat_kdfontop_ioctl(struct compat_console_font_op __user *fontop,
+			 int perm, struct console_font_op *op, struct vc_data *vc)
+{
+	int i;
+
+	if (copy_from_user(op, fontop, sizeof(struct compat_console_font_op)))
+		return -EFAULT;
+	if (!perm && op->op != KD_FONT_OP_GET)
+		return -EPERM;
+	op->data = compat_ptr(((struct compat_console_font_op *)op)->data);
+	op->flags |= KD_FONT_FLAG_OLD;
+	i = con_font_op(vc, op);
+	if (i)
+		return i;
+	((struct compat_console_font_op *)op)->data = (unsigned long)op->data;
+	if (copy_to_user(fontop, op, sizeof(struct compat_console_font_op)))
+		return -EFAULT;
+	return 0;
+}
+
+struct compat_unimapdesc {
+	unsigned short entry_ct;
+	compat_caddr_t entries;
+};
+
+static inline int
+compat_unimap_ioctl(unsigned int cmd, struct compat_unimapdesc __user *user_ud,
+			 int perm, struct vc_data *vc)
+{
+	struct compat_unimapdesc tmp;
+	struct unipair __user *tmp_entries;
+
+	if (copy_from_user(&tmp, user_ud, sizeof tmp))
+		return -EFAULT;
+	tmp_entries = compat_ptr(tmp.entries);
+	if (tmp_entries)
+		if (!access_ok(VERIFY_WRITE, tmp_entries,
+				tmp.entry_ct*sizeof(struct unipair)))
+			return -EFAULT;
+	switch (cmd) {
+	case PIO_UNIMAP:
+		if (!perm)
+			return -EPERM;
+		return con_set_unimap(vc, tmp.entry_ct, tmp_entries);
+	case GIO_UNIMAP:
+		if (!perm && fg_console != vc->vc_num)
+			return -EPERM;
+		return con_get_unimap(vc, tmp.entry_ct, &(user_ud->entry_ct), tmp_entries);
+	}
+	return 0;
+}
+
+long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
+	     unsigned int cmd, unsigned long arg)
+{
+	struct vc_data *vc = tty->driver_data;
+	struct console_font_op op;	/* used in multiple places here */
+	struct kbd_struct *kbd;
+	unsigned int console;
+	void __user *up = (void __user *)arg;
+	int perm;
+	int ret = 0;
+
+	console = vc->vc_num;
+
+	lock_kernel();
+
+	if (!vc_cons_allocated(console)) { 	/* impossible? */
+		ret = -ENOIOCTLCMD;
+		goto out;
+	}
+
+	/*
+	 * To have permissions to do most of the vt ioctls, we either have
+	 * to be the owner of the tty, or have CAP_SYS_TTY_CONFIG.
+	 */
+	perm = 0;
+	if (current->signal->tty == tty || capable(CAP_SYS_TTY_CONFIG))
+		perm = 1;
+
+	kbd = kbd_table + console;
+	switch (cmd) {
+	/*
+	 * these need special handlers for incompatible data structures
+	 */
+	case PIO_FONTX:
+	case GIO_FONTX:
+		ret = compat_fontx_ioctl(cmd, up, perm, &op);
+		break;
+
+	case KDFONTOP:
+		ret = compat_kdfontop_ioctl(up, perm, &op, vc);
+		break;
+
+	case PIO_UNIMAP:
+	case GIO_UNIMAP:
+		ret = do_unimap_ioctl(cmd, up, perm, vc);
+		break;
+
+	/*
+	 * all these treat 'arg' as an integer
+	 */
+	case KIOCSOUND:
+	case KDMKTONE:
+#ifdef CONFIG_X86
+	case KDADDIO:
+	case KDDELIO:
+#endif
+	case KDSETMODE:
+	case KDMAPDISP:
+	case KDUNMAPDISP:
+	case KDSKBMODE:
+	case KDSKBMETA:
+	case KDSKBLED:
+	case KDSETLED:
+	case KDSIGACCEPT:
+	case VT_ACTIVATE:
+	case VT_WAITACTIVE:
+	case VT_RELDISP:
+	case VT_DISALLOCATE:
+	case VT_RESIZE:
+	case VT_RESIZEX:
+		goto fallback;
+
+	/*
+	 * the rest has a compatible data structure behind arg,
+	 * but we have to convert it to a proper 64 bit pointer.
+	 */
+	default:
+		arg = (unsigned long)compat_ptr(arg);
+		goto fallback;
+	}
+out:
+	unlock_kernel();
+	return ret;
+
+fallback:
+	unlock_kernel();
+	return vt_ioctl(tty, file, cmd, arg);
+}
+
+
+#endif /* CONFIG_COMPAT */
+
+
 /*
  * Sometimes we want to wait until a particular VT has been activated. We
  * do it in a very simple manner. Everybody waits on a single queue and
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..38f3786 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -526,5 +526,8 @@ extern void console_print(const char *);
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
 		    unsigned int cmd, unsigned long arg);
 
+extern long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
+		     unsigned int cmd, unsigned long arg);
+
 #endif /* __KERNEL__ */
 #endif
-- 
1.6.3.3

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Frederic Weisbecker]

Hi,


On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote:
> The VT specific compat_ioctl handlers are the only ones
> in common code that require the BKL. Moving them into
> the vt driver lets us remove the BKL from the other handlers
> and cleans up the code.


Why does it require the bkl?


> +
> +long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
> +	     unsigned int cmd, unsigned long arg)
> +{
> +	struct vc_data *vc = tty->driver_data;
> +	struct console_font_op op;	/* used in multiple places here */
> +	struct kbd_struct *kbd;
> +	unsigned int console;
> +	void __user *up = (void __user *)arg;
> +	int perm;
> +	int ret = 0;
> +
> +	console = vc->vc_num;
> +
> +	lock_kernel();



It would be really nice to add a comment here that explain what it
is protecting.
I would like to work on removing the bkl from tty, and such nude lock_kernel()
don't help much to work in this area.
This is more than ever a FUD lock, nothing about its role in tty in the Lockronomicon,
and even not in the comments :-)

Thanks!
Frederic.


> +
> +	if (!vc_cons_allocated(console)) { 	/* impossible? */
> +		ret = -ENOIOCTLCMD;
> +		goto out;
> +	}
> +
> +	/*
> +	 * To have permissions to do most of the vt ioctls, we either have
> +	 * to be the owner of the tty, or have CAP_SYS_TTY_CONFIG.
> +	 */
> +	perm = 0;
> +	if (current->signal->tty == tty || capable(CAP_SYS_TTY_CONFIG))
> +		perm = 1;
> +
> +	kbd = kbd_table + console;
> +	switch (cmd) {
> +	/*
> +	 * these need special handlers for incompatible data structures
> +	 */
> +	case PIO_FONTX:
> +	case GIO_FONTX:
> +		ret = compat_fontx_ioctl(cmd, up, perm, &op);
> +		break;
> +
> +	case KDFONTOP:
> +		ret = compat_kdfontop_ioctl(up, perm, &op, vc);
> +		break;
> +
> +	case PIO_UNIMAP:
> +	case GIO_UNIMAP:
> +		ret = do_unimap_ioctl(cmd, up, perm, vc);
> +		break;
> +
> +	/*
> +	 * all these treat 'arg' as an integer
> +	 */
> +	case KIOCSOUND:
> +	case KDMKTONE:
> +#ifdef CONFIG_X86
> +	case KDADDIO:
> +	case KDDELIO:
> +#endif
> +	case KDSETMODE:
> +	case KDMAPDISP:
> +	case KDUNMAPDISP:
> +	case KDSKBMODE:
> +	case KDSKBMETA:
> +	case KDSKBLED:
> +	case KDSETLED:
> +	case KDSIGACCEPT:
> +	case VT_ACTIVATE:
> +	case VT_WAITACTIVE:
> +	case VT_RELDISP:
> +	case VT_DISALLOCATE:
> +	case VT_RESIZE:
> +	case VT_RESIZEX:
> +		goto fallback;
> +
> +	/*
> +	 * the rest has a compatible data structure behind arg,
> +	 * but we have to convert it to a proper 64 bit pointer.
> +	 */
> +	default:
> +		arg = (unsigned long)compat_ptr(arg);
> +		goto fallback;
> +	}
> +out:
> +	unlock_kernel();
> +	return ret;
> +
> +fallback:
> +	unlock_kernel();
> +	return vt_ioctl(tty, file, cmd, arg);
> +}
> +
> +
> +#endif /* CONFIG_COMPAT */
> +
> +
>  /*
>   * Sometimes we want to wait until a particular VT has been activated. We
>   * do it in a very simple manner. Everybody waits on a single queue and
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index e8c6c91..38f3786 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -526,5 +526,8 @@ extern void console_print(const char *);
>  extern int vt_ioctl(struct tty_struct *tty, struct file *file,
>  		    unsigned int cmd, unsigned long arg);
>  
> +extern long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
> +		     unsigned int cmd, unsigned long arg);
> +
>  #endif /* __KERNEL__ */
>  #endif
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Arnd Bergmann]

On Friday 07 August 2009, Frederic Weisbecker wrote:

> On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote:
> > The VT specific compat_ioctl handlers are the only ones
> > in common code that require the BKL. Moving them into
> > the vt driver lets us remove the BKL from the other handlers
> > and cleans up the code.
> 
> 
> Why does it require the bkl?
> 

All the VT ioctls are currently called under the BKL. I did not try
to find out why that is but simply kept that state. All other
compat ioctl do not interact with device driver state at all,
so they obviously do not need the BKL.

> > +
> > +long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
> > +	     unsigned int cmd, unsigned long arg)
> > +{
> > +	struct vc_data *vc = tty->driver_data;
> > +	struct console_font_op op;	/* used in multiple places here */
> > +	struct kbd_struct *kbd;
> > +	unsigned int console;
> > +	void __user *up = (void __user *)arg;
> > +	int perm;
> > +	int ret = 0;
> > +
> > +	console = vc->vc_num;
> > +
> > +	lock_kernel();
> 
> 
> 
> It would be really nice to add a comment here that explain what it
> is protecting.
> I would like to work on removing the bkl from tty, and such nude lock_kernel()
> don't help much to work in this area.
> This is more than ever a FUD lock, nothing about its role in tty in the Lockronomicon,
> and even not in the comments :-)

This function is a straight copy from vt_ioctl, with the data structures replaced,
and calling it vt_ioctl where they are identical.

You are right that this needs more code comments to make that obvious.

	Arnd <><

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 09:04:11AM +0200, Arnd Bergmann wrote:
> On Friday 07 August 2009, Frederic Weisbecker wrote:
> 
> > On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote:
> > > The VT specific compat_ioctl handlers are the only ones
> > > in common code that require the BKL. Moving them into
> > > the vt driver lets us remove the BKL from the other handlers
> > > and cleans up the code.
> > 
> > 
> > Why does it require the bkl?
> > 
> 
> All the VT ioctls are currently called under the BKL. I did not try
> to find out why that is but simply kept that state. All other
> compat ioctl do not interact with device driver state at all,
> so they obviously do not need the BKL.


Ah ok.


 
> > > +
> > > +long vt_compat_ioctl(struct tty_struct *tty, struct file * file,
> > > +	     unsigned int cmd, unsigned long arg)
> > > +{
> > > +	struct vc_data *vc = tty->driver_data;
> > > +	struct console_font_op op;	/* used in multiple places here */
> > > +	struct kbd_struct *kbd;
> > > +	unsigned int console;
> > > +	void __user *up = (void __user *)arg;
> > > +	int perm;
> > > +	int ret = 0;
> > > +
> > > +	console = vc->vc_num;
> > > +
> > > +	lock_kernel();
> > 
> > 
> > 
> > It would be really nice to add a comment here that explain what it
> > is protecting.
> > I would like to work on removing the bkl from tty, and such nude lock_kernel()
> > don't help much to work in this area.
> > This is more than ever a FUD lock, nothing about its role in tty in the Lockronomicon,
> > and even not in the comments :-)
> 
> This function is a straight copy from vt_ioctl, with the data structures replaced,
> and calling it vt_ioctl where they are identical.
> 
> You are right that this needs more code comments to make that obvious.
> 
> 	Arnd <><


Ok. This looks like a nice series. A bkl pushdown that only goes down
in one site among several others enlightens the understanding of what it
is protecting (beside the nice fact it also burned three bkl callsites :-)

Thanks!

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Arnd Bergmann]

On Friday 07 August 2009, Frederic Weisbecker wrote:
> Ok. This looks like a nice series. A bkl pushdown that only goes down
> in one site among several others enlightens the understanding of what it
> is protecting (beside the nice fact it also burned three bkl callsites :-)

Thanks!

Well, most importantly patch 5/5 fixes a long-standing bug where we held
the BKL in lots of places that were already proven not to need it, or
alternatively held it twice (nested) in the ioctls that may still need it.

I did patch 3/5 this way because I read that you were working on BKL
removal for TTY and wanted to do my share by removing the dependency
on the code that I care about (fs/compat_ioctl.h).

	Arnd <><

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 02:02:11PM +0200, Arnd Bergmann wrote:
> On Friday 07 August 2009, Frederic Weisbecker wrote:
> > Ok. This looks like a nice series. A bkl pushdown that only goes down
> > in one site among several others enlightens the understanding of what it
> > is protecting (beside the nice fact it also burned three bkl callsites :-)
> 
> Thanks!
> 
> Well, most importantly patch 5/5 fixes a long-standing bug where we held
> the BKL in lots of places that were already proven not to need it, or
> alternatively held it twice (nested) in the ioctls that may still need it.
> 
> I did patch 3/5 this way because I read that you were working on BKL
> removal for TTY and wanted to do my share by removing the dependency
> on the code that I care about (fs/compat_ioctl.h).
> 
> 	Arnd <><

Thanks!

I guess these patches could fit in the tty tree, unless someone has
objections?

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Greg KH]

On Sat, Aug 08, 2009 at 02:34:53AM +0200, Frederic Weisbecker wrote:
> On Fri, Aug 07, 2009 at 02:02:11PM +0200, Arnd Bergmann wrote:
> > On Friday 07 August 2009, Frederic Weisbecker wrote:
> > > Ok. This looks like a nice series. A bkl pushdown that only goes down
> > > in one site among several others enlightens the understanding of what it
> > > is protecting (beside the nice fact it also burned three bkl callsites :-)
> > 
> > Thanks!
> > 
> > Well, most importantly patch 5/5 fixes a long-standing bug where we held
> > the BKL in lots of places that were already proven not to need it, or
> > alternatively held it twice (nested) in the ioctls that may still need it.
> > 
> > I did patch 3/5 this way because I read that you were working on BKL
> > removal for TTY and wanted to do my share by removing the dependency
> > on the code that I care about (fs/compat_ioctl.h).
> > 
> > 	Arnd <><
> 
> Thanks!
> 
> I guess these patches could fit in the tty tree, unless someone has
> objections?

I've taken this patch, I didn't see any others sent to me :(

thanks,

greg k-h

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 05:41:56PM -0700, Greg KH wrote:
> On Sat, Aug 08, 2009 at 02:34:53AM +0200, Frederic Weisbecker wrote:
> > On Fri, Aug 07, 2009 at 02:02:11PM +0200, Arnd Bergmann wrote:
> > > On Friday 07 August 2009, Frederic Weisbecker wrote:
> > > > Ok. This looks like a nice series. A bkl pushdown that only goes down
> > > > in one site among several others enlightens the understanding of what it
> > > > is protecting (beside the nice fact it also burned three bkl callsites :-)
> > > 
> > > Thanks!
> > > 
> > > Well, most importantly patch 5/5 fixes a long-standing bug where we held
> > > the BKL in lots of places that were already proven not to need it, or
> > > alternatively held it twice (nested) in the ioctls that may still need it.
> > > 
> > > I did patch 3/5 this way because I read that you were working on BKL
> > > removal for TTY and wanted to do my share by removing the dependency
> > > on the code that I care about (fs/compat_ioctl.h).
> > > 
> > > 	Arnd <><
> > 
> > Thanks!
> > 
> > I guess these patches could fit in the tty tree, unless someone has
> > objections?
> 
> I've taken this patch, I didn't see any others sent to me :(
> 
> thanks,
> 
> greg k-h


Hmm, several subsystems are involved in this patchset.
But another one is concerned by the tty tree:

[PATCH 2/5] s390: move keyboard compat ioctls into tty3270 driver

But this whole patchset seems broken if it gets disseminated in several
trees.
May be the whole patchset can go in yours?

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Greg KH]

On Sat, Aug 08, 2009 at 03:03:36AM +0200, Frederic Weisbecker wrote:
> On Fri, Aug 07, 2009 at 05:41:56PM -0700, Greg KH wrote:
> > On Sat, Aug 08, 2009 at 02:34:53AM +0200, Frederic Weisbecker wrote:
> > > On Fri, Aug 07, 2009 at 02:02:11PM +0200, Arnd Bergmann wrote:
> > > > On Friday 07 August 2009, Frederic Weisbecker wrote:
> > > > > Ok. This looks like a nice series. A bkl pushdown that only goes down
> > > > > in one site among several others enlightens the understanding of what it
> > > > > is protecting (beside the nice fact it also burned three bkl callsites :-)
> > > > 
> > > > Thanks!
> > > > 
> > > > Well, most importantly patch 5/5 fixes a long-standing bug where we held
> > > > the BKL in lots of places that were already proven not to need it, or
> > > > alternatively held it twice (nested) in the ioctls that may still need it.
> > > > 
> > > > I did patch 3/5 this way because I read that you were working on BKL
> > > > removal for TTY and wanted to do my share by removing the dependency
> > > > on the code that I care about (fs/compat_ioctl.h).
> > > > 
> > > > 	Arnd <><
> > > 
> > > Thanks!
> > > 
> > > I guess these patches could fit in the tty tree, unless someone has
> > > objections?
> > 
> > I've taken this patch, I didn't see any others sent to me :(
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> Hmm, several subsystems are involved in this patchset.
> But another one is concerned by the tty tree:
> 
> [PATCH 2/5] s390: move keyboard compat ioctls into tty3270 driver
> 
> But this whole patchset seems broken if it gets disseminated in several
> trees.
> May be the whole patchset can go in yours?

sure, someone want to send them all to me after getting the other
subsystem's acks?

thanks,

greg k-h

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Arnd Bergmann]

On Saturday 08 August 2009, Greg KH wrote:
> sure, someone want to send them all to me after getting the other
> subsystem's acks?

Yes, sounds good. 

Martin and Jeff, could you have a look at the first two patches?

The final patch is not tty specific, but it still depends on
the others, so I guess you can take that one as well. It could
use an extra pair of eyeballs though, just to see if my logic
was correct, the impact should be rather significant.

	Arnd <><

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Alan Cox]

On Fri, 7 Aug 2009 08:23:58 +0200
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Hi,
> 
> 
> On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote:
> > The VT specific compat_ioctl handlers are the only ones
> > in common code that require the BKL. Moving them into
> > the vt driver lets us remove the BKL from the other handlers
> > and cleans up the code.
> 
> 
> Why does it require the bkl?

It's always taken the BKL - you have to prove it doesn't need it. Which
btw isn't true - it does need it in various places still.

Alan

Re: [PATCH 3/5] tty: handle VT specific compat ioctls in vt driver

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 10:57:32AM +0100, Alan Cox wrote:
> On Fri, 7 Aug 2009 08:23:58 +0200
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Hi,
> > 
> > 
> > On Thu, Aug 06, 2009 at 03:09:28PM +0200, Arnd Bergmann wrote:
> > > The VT specific compat_ioctl handlers are the only ones
> > > in common code that require the BKL. Moving them into
> > > the vt driver lets us remove the BKL from the other handlers
> > > and cleans up the code.
> > 
> > 
> > Why does it require the bkl?
> 
> It's always taken the BKL - you have to prove it doesn't need it. Which
> btw isn't true - it does need it in various places still.
> 
> Alan


It was a way to tell "I would like to know what it is protecting" ;-)
I can imagine it is not here for no reason, the problem is to find why.

As an example, to find the reason of the following lines in do_tty_hangup():

	/* inuse_filps is protected by the single kernel lock */
	lock_kernel();

I had to look at a 2.2 kernel. At this time, inuse_filps existed,
and now it is replaced by the tty->tty_files field, which
is protected by file_list_lock().

So according to the comment, we can remove the bkl there, but what
guarantees its role hasn't evolved since then to make it protecting
something else...

[PATCH 4/5] compat_ioctl: remove VT specific ioctl handlers

[Arnd Bergmann]

The VT, tty3270 and UML console drivers now handle all of
these ioctls directly, so we can remove the handlers from
common code.

These are the only handlers that require the BKL because they
directly perform the ioctl action rather than just converting
the data structures. Once they are gone, we can remove the
BKL from the remaining ioctl conversion handlers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/compat_ioctl.c |  211 -----------------------------------------------------
 1 files changed, 0 insertions(+), 211 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index f28f070..5d800aa 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1039,161 +1039,6 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
 
 #endif /* CONFIG_BLOCK */
 
-#ifdef CONFIG_VT
-
-static int vt_check(struct file *file)
-{
-	struct tty_struct *tty;
-	struct inode *inode = file->f_path.dentry->d_inode;
-	struct vc_data *vc;
-	
-	if (file->f_op->unlocked_ioctl != tty_ioctl)
-		return -EINVAL;
-	                
-	tty = (struct tty_struct *)file->private_data;
-	if (tty_paranoia_check(tty, inode, "tty_ioctl"))
-		return -EINVAL;
-	                                                
-	if (tty->ops->ioctl != vt_ioctl)
-		return -EINVAL;
-
-	vc = (struct vc_data *)tty->driver_data;
-	if (!vc_cons_allocated(vc->vc_num)) 	/* impossible? */
-		return -ENOIOCTLCMD;
-
-	/*
-	 * To have permissions to do most of the vt ioctls, we either have
-	 * to be the owner of the tty, or have CAP_SYS_TTY_CONFIG.
-	 */
-	if (current->signal->tty == tty || capable(CAP_SYS_TTY_CONFIG))
-		return 1;
-	return 0;                                                    
-}
-
-struct consolefontdesc32 {
-	unsigned short charcount;       /* characters in font (256 or 512) */
-	unsigned short charheight;      /* scan lines per character (1-32) */
-	compat_caddr_t chardata;	/* font data in expanded form */
-};
-
-static int do_fontx_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct consolefontdesc32 __user *user_cfd = compat_ptr(arg);
-	struct console_font_op op;
-	compat_caddr_t data;
-	int i, perm;
-
-	perm = vt_check(file);
-	if (perm < 0) return perm;
-	
-	switch (cmd) {
-	case PIO_FONTX:
-		if (!perm)
-			return -EPERM;
-		op.op = KD_FONT_OP_SET;
-		op.flags = 0;
-		op.width = 8;
-		if (get_user(op.height, &user_cfd->charheight) ||
-		    get_user(op.charcount, &user_cfd->charcount) ||
-		    get_user(data, &user_cfd->chardata))
-			return -EFAULT;
-		op.data = compat_ptr(data);
-		return con_font_op(vc_cons[fg_console].d, &op);
-	case GIO_FONTX:
-		op.op = KD_FONT_OP_GET;
-		op.flags = 0;
-		op.width = 8;
-		if (get_user(op.height, &user_cfd->charheight) ||
-		    get_user(op.charcount, &user_cfd->charcount) ||
-		    get_user(data, &user_cfd->chardata))
-			return -EFAULT;
-		if (!data)
-			return 0;
-		op.data = compat_ptr(data);
-		i = con_font_op(vc_cons[fg_console].d, &op);
-		if (i)
-			return i;
-		if (put_user(op.height, &user_cfd->charheight) ||
-		    put_user(op.charcount, &user_cfd->charcount) ||
-		    put_user((compat_caddr_t)(unsigned long)op.data,
-				&user_cfd->chardata))
-			return -EFAULT;
-		return 0;
-	}
-	return -EINVAL;
-}
-
-struct console_font_op32 {
-	compat_uint_t op;        /* operation code KD_FONT_OP_* */
-	compat_uint_t flags;     /* KD_FONT_FLAG_* */
-	compat_uint_t width, height;     /* font size */
-	compat_uint_t charcount;
-	compat_caddr_t data;    /* font data with height fixed to 32 */
-};
-                                        
-static int do_kdfontop_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct console_font_op op;
-	struct console_font_op32 __user *fontop = compat_ptr(arg);
-	int perm = vt_check(file), i;
-	struct vc_data *vc;
-	
-	if (perm < 0) return perm;
-	
-	if (copy_from_user(&op, fontop, sizeof(struct console_font_op32)))
-		return -EFAULT;
-	if (!perm && op.op != KD_FONT_OP_GET)
-		return -EPERM;
-	op.data = compat_ptr(((struct console_font_op32 *)&op)->data);
-	op.flags |= KD_FONT_FLAG_OLD;
-	vc = ((struct tty_struct *)file->private_data)->driver_data;
-	i = con_font_op(vc, &op);
-	if (i)
-		return i;
-	((struct console_font_op32 *)&op)->data = (unsigned long)op.data;
-	if (copy_to_user(fontop, &op, sizeof(struct console_font_op32)))
-		return -EFAULT;
-	return 0;
-}
-
-struct unimapdesc32 {
-	unsigned short entry_ct;
-	compat_caddr_t entries;
-};
-
-static int do_unimap_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct unimapdesc32 tmp;
-	struct unimapdesc32 __user *user_ud = compat_ptr(arg);
-	int perm = vt_check(file);
-	struct vc_data *vc;
-
-	if (perm < 0)
-		return perm;
-	if (copy_from_user(&tmp, user_ud, sizeof tmp))
-		return -EFAULT;
-	if (tmp.entries)
-		if (!access_ok(VERIFY_WRITE, compat_ptr(tmp.entries),
-				tmp.entry_ct*sizeof(struct unipair)))
-			return -EFAULT;
-	vc = ((struct tty_struct *)file->private_data)->driver_data;
-	switch (cmd) {
-	case PIO_UNIMAP:
-		if (!perm)
-			return -EPERM;
-		return con_set_unimap(vc, tmp.entry_ct,
-						compat_ptr(tmp.entries));
-	case GIO_UNIMAP:
-		if (!perm && fg_console != vc->vc_num)
-			return -EPERM;
-		return con_get_unimap(vc, tmp.entry_ct, &(user_ud->entry_ct),
-						compat_ptr(tmp.entries));
-	}
-	return 0;
-}
-
-#endif /* CONFIG_VT */
-
 static int do_smb_getmountuid(unsigned int fd, unsigned int cmd, unsigned long arg)
 {
 	mm_segment_t old_fs = get_fs();
@@ -1864,7 +1709,6 @@ COMPATIBLE_IOCTL(TCGETS)
 COMPATIBLE_IOCTL(TCSETS)
 COMPATIBLE_IOCTL(TCSETSW)
 COMPATIBLE_IOCTL(TCSETSF)
-COMPATIBLE_IOCTL(TIOCLINUX)
 COMPATIBLE_IOCTL(TIOCSBRK)
 COMPATIBLE_IOCTL(TIOCCBRK)
 ULONG_IOCTL(TIOCMIWAIT)
@@ -1933,40 +1777,6 @@ COMPATIBLE_IOCTL(STOP_ARRAY_RO)
 COMPATIBLE_IOCTL(RESTART_ARRAY_RW)
 COMPATIBLE_IOCTL(GET_BITMAP_FILE)
 ULONG_IOCTL(SET_BITMAP_FILE)
-/* Big K */
-COMPATIBLE_IOCTL(PIO_FONT)
-COMPATIBLE_IOCTL(GIO_FONT)
-COMPATIBLE_IOCTL(PIO_CMAP)
-COMPATIBLE_IOCTL(GIO_CMAP)
-ULONG_IOCTL(KDSIGACCEPT)
-COMPATIBLE_IOCTL(KDGETKEYCODE)
-COMPATIBLE_IOCTL(KDSETKEYCODE)
-ULONG_IOCTL(KIOCSOUND)
-ULONG_IOCTL(KDMKTONE)
-COMPATIBLE_IOCTL(KDGKBTYPE)
-ULONG_IOCTL(KDSETMODE)
-COMPATIBLE_IOCTL(KDGETMODE)
-ULONG_IOCTL(KDSKBMODE)
-COMPATIBLE_IOCTL(KDGKBMODE)
-ULONG_IOCTL(KDSKBMETA)
-COMPATIBLE_IOCTL(KDGKBMETA)
-COMPATIBLE_IOCTL(KDGKBENT)
-COMPATIBLE_IOCTL(KDSKBENT)
-COMPATIBLE_IOCTL(KDGKBSENT)
-COMPATIBLE_IOCTL(KDSKBSENT)
-COMPATIBLE_IOCTL(KDGKBDIACR)
-COMPATIBLE_IOCTL(KDSKBDIACR)
-COMPATIBLE_IOCTL(KDKBDREP)
-COMPATIBLE_IOCTL(KDGKBLED)
-ULONG_IOCTL(KDSKBLED)
-COMPATIBLE_IOCTL(KDGETLED)
-ULONG_IOCTL(KDSETLED)
-COMPATIBLE_IOCTL(GIO_SCRNMAP)
-COMPATIBLE_IOCTL(PIO_SCRNMAP)
-COMPATIBLE_IOCTL(GIO_UNISCRNMAP)
-COMPATIBLE_IOCTL(PIO_UNISCRNMAP)
-COMPATIBLE_IOCTL(PIO_FONTRESET)
-COMPATIBLE_IOCTL(PIO_UNIMAPCLR)
 #ifdef CONFIG_BLOCK
 /* Big S */
 COMPATIBLE_IOCTL(SCSI_IOCTL_GET_IDLUN)
@@ -1990,20 +1800,6 @@ COMPATIBLE_IOCTL(TUNSETOFFLOAD)
 COMPATIBLE_IOCTL(TUNSETTXFILTER)
 COMPATIBLE_IOCTL(TUNGETSNDBUF)
 COMPATIBLE_IOCTL(TUNSETSNDBUF)
-/* Big V */
-COMPATIBLE_IOCTL(VT_SETMODE)
-COMPATIBLE_IOCTL(VT_GETMODE)
-COMPATIBLE_IOCTL(VT_GETSTATE)
-COMPATIBLE_IOCTL(VT_OPENQRY)
-ULONG_IOCTL(VT_ACTIVATE)
-ULONG_IOCTL(VT_WAITACTIVE)
-ULONG_IOCTL(VT_RELDISP)
-ULONG_IOCTL(VT_DISALLOCATE)
-COMPATIBLE_IOCTL(VT_RESIZE)
-COMPATIBLE_IOCTL(VT_RESIZEX)
-COMPATIBLE_IOCTL(VT_LOCKSWITCH)
-COMPATIBLE_IOCTL(VT_UNLOCKSWITCH)
-COMPATIBLE_IOCTL(VT_GETHIFONTMASK)
 /* Little p (/dev/rtc, /dev/envctrl, etc.) */
 COMPATIBLE_IOCTL(RTC_AIE_ON)
 COMPATIBLE_IOCTL(RTC_AIE_OFF)
@@ -2602,13 +2398,6 @@ HANDLE_IOCTL(MTIOCPOS32, mt_ioctl_trans)
 #endif
 #define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
 HANDLE_IOCTL(AUTOFS_IOC_SETTIMEOUT32, ioc_settimeout)
-#ifdef CONFIG_VT
-HANDLE_IOCTL(PIO_FONTX, do_fontx_ioctl)
-HANDLE_IOCTL(GIO_FONTX, do_fontx_ioctl)
-HANDLE_IOCTL(PIO_UNIMAP, do_unimap_ioctl)
-HANDLE_IOCTL(GIO_UNIMAP, do_unimap_ioctl)
-HANDLE_IOCTL(KDFONTOP, do_kdfontop_ioctl)
-#endif
 /* One SMB ioctl needs translations. */
 #define SMB_IOC_GETMOUNTUID_32 _IOR('u', 1, compat_uid_t)
 HANDLE_IOCTL(SMB_IOC_GETMOUNTUID_32, do_smb_getmountuid)
-- 
1.6.3.3

[PATCH 5/5] compat_ioctl: do not hold BKL in handlers

[Arnd Bergmann]

We have always called ioctl conversion handlers under the big
kernel lock, although that is generally not necessary. In particular
it is not needed for conversion of data structures and for calling
sys_ioctl or do_vfs_ioctl, which will get the BKL again if needed.

Handlers doing more than those two have been moved out, so we
can kill off the BKL from compat_sys_ioctl. This may significantly
improve latencies with 32 bit applications, and it avoids a common
scenario where a thread acquires the BKL twice.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/compat_ioctl.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 5d800aa..86e6c75 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2637,9 +2637,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 
  found_handler:
 	if (t->handler) {
-		lock_kernel();
 		error = t->handler(fd, cmd, arg, filp);
-		unlock_kernel();
 		goto out_fput;
 	}
 
-- 
1.6.3.3