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

Frederic Weisbecker
2009-08-07T06:24:04Z

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/