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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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