Re: [PATCH v2] x86: fix ktermios-termio conversion

[Ingo Molnar <mingo@elte.hu>]

* Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 12 May 2009, Ingo Molnar wrote:
> > Hm, that looks a bit ugly and it also adds 150 bytes of bloat to the 
> > kernel:
> > 
> > drivers/char/tty_ioctl.o:
> > 
> >    text    data     bss     dec     hex filename
> >    4704       0       0    4704    1260 tty_ioctl.o.before
> >    4841       0       0    4841    12e9 tty_ioctl.o.after
> 
> It's easy to go back to a macro (or replace it by another inline 
> function if you prefer that stylistically) to make it look nicer.

no, we shouldnt go back to a macro.

> Regarding the bloat, I'm still looking for a better version. The 
> code I posted is basically what is in asm-generic/termios.h 
> traditionally, so at least I'm reasonably confident that it is 
> correct, while the x86 version currently fails to return -EFAULT 
> on incorrect pointers and misses c_line changes.
> 
> I now tried this version, which theoretically should be fairly 
> compact on most architectures:
> 
> static inline int set_low_termios_bits(unsigned int *termios,
> 			const short __user *termio)
> {
> 	unsigned short tmp;
> 	int ret;
> 
> 	ret = __get_user(tmp, termio);
> 	*termios = (0xffff0000 & *termios) | tmp;
> 
> 	return ret;
> }
> 
> /*
>  * Translate a "termio" structure into a "termios". Ugh.
>  */
> static inline int user_termio_to_kernel_termios(struct ktermios *termios,
> 						const struct termio __user *termio)
> {
> 	if (access_ok(VERIFY_READ, termio, sizeof (*termio)))
> 		return -EFAULT;
> 
> 	return set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) |
> 	       set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) |
> 	       set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) |
> 	       set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) |
> 	       __get_user(termios->c_line, &termio->c_line) |
> 	       (__copy_from_user(termios->c_cc, termio->c_cc, NCC)
> 				? -EFAULT : 0);
> }
> 
> /*
>  * Translate a "termios" structure into a "termio". Ugh.
>  */
> static inline int kernel_termios_to_user_termio(struct termio __user *termio,
> 						struct ktermios *termios)
> {
> 	if (access_ok(VERIFY_WRITE, termio, sizeof (*termio)))
> 		return -EFAULT;
> 
> 	return __put_user(termios->c_iflag, &termio->c_iflag) |
> 	       __put_user(termios->c_oflag, &termio->c_oflag) |
> 	       __put_user(termios->c_cflag, &termio->c_cflag) |
> 	       __put_user(termios->c_lflag, &termio->c_lflag) |
> 	       __put_user(termios->c_line,  &termio->c_line) |
> 	       (__copy_to_user(termio->c_cc, termios->c_cc, NCC)
> 				? -EFAULT : 0);
> }
> 
> Unfortunately, this is even more code on x86, where get_user/put_user
> is out-of-line, while __get_user/__put_user is inline and produces
> fixup code.
> The best I could come up with is somewhat slower but also shorter:
> 
> static inline int set_low_termios_bits(unsigned int *termios,
> 			const short __user *termio)
> {
> 	unsigned short tmp;
> 	int ret;
> 
> 	ret = get_user(tmp, termio);
> 	*termios = (0xffff0000 & *termios) | tmp;
> 
> 	return ret;
> }
> 
> /*
>  * Translate a "termio" structure into a "termios". Ugh.
>  */
> static inline int user_termio_to_kernel_termios(struct ktermios *termios,
> 						const struct termio __user *termio)
> {
> 	return (set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) ||
> 		set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) ||
> 		set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) ||
> 		set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) ||
> 		get_user(termios->c_line, &termio->c_line) ||
> 		copy_from_user(termios->c_cc, termio->c_cc, NCC))
> 				? -EFAULT : 0;
> }
> 
> /*
>  * Translate a "termios" structure into a "termio". Ugh.
>  */
> static inline int kernel_termios_to_user_termio(struct termio __user *termio,
> 						struct ktermios *termios)
> {
> 	return put_user(termios->c_iflag, &termio->c_iflag) ||
> 	       put_user(termios->c_oflag, &termio->c_oflag) ||
> 	       put_user(termios->c_cflag, &termio->c_cflag) ||
> 	       put_user(termios->c_lflag, &termio->c_lflag) ||
> 	       put_user(termios->c_line,  &termio->c_line) ||
> 	       (copy_to_user(termio->c_cc, termios->c_cc, NCC))
> 				? -EFAULT : 0;
> }
> 
> If you think that looks better, I can send another patch. I have 
> not tested this code functionally though.

hm, on x86 you could use get_user_try / get_user_ex() / 
get_user_catch() approach to linearize the dependencies and reduce 
register pressure.

	Ingo