From: Richard W. <ri...@no...> - 2012-06-04 20:27:53
|
This patch set moves the UML console driver to the new TTY port interface. It does ref counting and uses the tty_port_*-helpers. Please note, it's not yet UML mconsole safe! Anyway, I see some really strange things and I'm not sure whether my patch is sane or not... If I implement tty_operations->hangup() the following happens: FC12: Login on tty0 works fine. On all other ttys login works but bash dies because of of -EIO. After vhangup() the tty returns -EIO upon read()/write(). FC16: Login broken on all ttys (bash dies with EIO like on FC12). If I start UML with rootfs read-only login works on tty0. - WTF?! Debian 6.0: Login works perfectly fine on all ttys Without tty_operations->hangup() the following happens: FC12: Login on tty0 works fine. mingetty is unable to start on anything else than tty0. It exits after a few seconds. FC16: Unable to start any mingetty (like on FC12 it exits after a few seconds) With read-only rootfs mingetty starts at least on tty0 and login works. (Again, WTF?) Debian 6.0 Login works perfectly fine on all ttys. I have no idea what's the root cause of this, there seems to be a lot of black magic involved. Alan, do you think the issues are caused by Fedora's broken user space? How can we fix this? Thanks, //richard (wearing a voodoo priests robe) [PATCH 1/6] TTY: um/line, add tty_port [PATCH 2/6] TTY: um/line, use tty from tty_port [PATCH 3/6] um: remove line_ioctl() [PATCH 4/6] um: Remove dead code [PATCH 5/6] um: fully use tty_port [PATCH 6/6] um: remove count_lock |
From: Richard W. <ri...@no...> - 2012-06-04 20:27:57
|
From: Jiri Slaby <js...@su...> This means switching to the tty refcounted model so that we will not race with interrupts. Signed-off-by: Jiri Slaby <js...@su...> Cc: Jeff Dike <jd...@ad...> Cc: Richard Weinberger <ri...@no...> Cc: use...@li... Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/chan_kern.c | 4 +++- arch/um/drivers/line.c | 25 ++++++++++++++++++------- arch/um/drivers/line.h | 1 - 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/arch/um/drivers/chan_kern.c b/arch/um/drivers/chan_kern.c index 45e248c..87eebfe 100644 --- a/arch/um/drivers/chan_kern.c +++ b/arch/um/drivers/chan_kern.c @@ -150,9 +150,11 @@ void chan_enable_winch(struct chan *chan, struct tty_struct *tty) static void line_timer_cb(struct work_struct *work) { struct line *line = container_of(work, struct line, task.work); + struct tty_struct *tty = tty_port_tty_get(&line->port); if (!line->throttled) - chan_interrupt(line, line->tty, line->driver->read_irq); + chan_interrupt(line, tty, line->driver->read_irq); + tty_kref_put(tty); } int enable_chan(struct line *line) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index 482a7bd..fb6e4ea 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -19,9 +19,11 @@ static irqreturn_t line_interrupt(int irq, void *data) { struct chan *chan = data; struct line *line = chan->line; + struct tty_struct *tty = tty_port_tty_get(&line->port); if (line) - chan_interrupt(line, line->tty, irq); + chan_interrupt(line, tty, irq); + tty_kref_put(tty); return IRQ_HANDLED; } @@ -333,7 +335,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data) { struct chan *chan = data; struct line *line = chan->line; - struct tty_struct *tty = line->tty; + struct tty_struct *tty; int err; /* @@ -352,10 +354,13 @@ static irqreturn_t line_write_interrupt(int irq, void *data) } spin_unlock(&line->lock); + tty = tty_port_tty_get(&line->port); if (tty == NULL) return IRQ_NONE; tty_wakeup(tty); + tty_kref_put(tty); + return IRQ_HANDLED; } @@ -409,7 +414,7 @@ int line_open(struct line *lines, struct tty_struct *tty) BUG_ON(tty->driver_data); tty->driver_data = line; - line->tty = tty; + tty_port_tty_set(&line->port, tty); err = enable_chan(line); if (err) /* line_close() will be called by our caller */ @@ -449,7 +454,7 @@ void line_close(struct tty_struct *tty, struct file * filp) if (--line->port.count) goto out_unlock; - line->tty = NULL; + tty_port_tty_set(&line->port, NULL); tty->driver_data = NULL; if (line->sigio) { @@ -610,9 +615,15 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str, mutex_lock(&line->count_lock); if (!line->valid) CONFIG_CHUNK(str, size, n, "none", 1); - else if (line->tty == NULL) - CONFIG_CHUNK(str, size, n, line->init_str, 1); - else n = chan_config_string(line, str, size, error_out); + else { + struct tty_struct *tty = tty_port_tty_get(&line->port); + if (tty == NULL) { + CONFIG_CHUNK(str, size, n, line->init_str, 1); + } else { + n = chan_config_string(line, str, size, error_out); + tty_kref_put(tty); + } + } mutex_unlock(&line->count_lock); return n; diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h index 0e06a1f..5b3d4fb 100644 --- a/arch/um/drivers/line.h +++ b/arch/um/drivers/line.h @@ -33,7 +33,6 @@ struct line_driver { struct line { struct tty_port port; - struct tty_struct *tty; struct mutex count_lock; int valid; -- 1.7.7.3 |
From: Richard W. <ri...@no...> - 2012-06-04 20:27:57
|
From: Jiri Slaby <js...@su...> And use count from there. Signed-off-by: Jiri Slaby <js...@su...> Cc: Jeff Dike <jd...@ad...> Cc: Richard Weinberger <ri...@no...> Cc: use...@li... Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/line.c | 7 ++++--- arch/um/drivers/line.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index acfd0e0..482a7bd 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -404,7 +404,7 @@ int line_open(struct line *lines, struct tty_struct *tty) goto out_unlock; err = 0; - if (line->count++) + if (line->port.count++) goto out_unlock; BUG_ON(tty->driver_data); @@ -446,7 +446,7 @@ void line_close(struct tty_struct *tty, struct file * filp) mutex_lock(&line->count_lock); BUG_ON(!line->valid); - if (--line->count) + if (--line->port.count) goto out_unlock; line->tty = NULL; @@ -478,7 +478,7 @@ int setup_one_line(struct line *lines, int n, char *init, mutex_lock(&line->count_lock); - if (line->count) { + if (line->port.count) { *error_out = "Device is already open"; goto out; } @@ -663,6 +663,7 @@ int register_lines(struct line_driver *line_driver, driver->init_termios = tty_std_termios; for (i = 0; i < nlines; i++) { + tty_port_init(&lines[i].port); spin_lock_init(&lines[i].lock); mutex_init(&lines[i].count_lock); lines[i].driver = line_driver; diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h index 0a18347..0e06a1f 100644 --- a/arch/um/drivers/line.h +++ b/arch/um/drivers/line.h @@ -32,9 +32,9 @@ struct line_driver { }; struct line { + struct tty_port port; struct tty_struct *tty; struct mutex count_lock; - unsigned long count; int valid; char *init_str; -- 1.7.7.3 |
From: Richard W. <ri...@no...> - 2012-06-04 20:27:58
|
line_ioctl() has no real function. Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/line.c | 86 --------------------------------------- arch/um/drivers/line.h | 2 - arch/um/drivers/ssl.c | 1 - arch/um/drivers/stdio_console.c | 1 - 4 files changed, 0 insertions(+), 90 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index fb6e4ea..131129a 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -221,92 +221,6 @@ void line_set_termios(struct tty_struct *tty, struct ktermios * old) /* nothing */ } -static const struct { - int cmd; - char *level; - char *name; -} tty_ioctls[] = { - /* don't print these, they flood the log ... */ - { TCGETS, NULL, "TCGETS" }, - { TCSETS, NULL, "TCSETS" }, - { TCSETSW, NULL, "TCSETSW" }, - { TCFLSH, NULL, "TCFLSH" }, - { TCSBRK, NULL, "TCSBRK" }, - - /* general tty stuff */ - { TCSETSF, KERN_DEBUG, "TCSETSF" }, - { TCGETA, KERN_DEBUG, "TCGETA" }, - { TIOCMGET, KERN_DEBUG, "TIOCMGET" }, - { TCSBRKP, KERN_DEBUG, "TCSBRKP" }, - { TIOCMSET, KERN_DEBUG, "TIOCMSET" }, - - /* linux-specific ones */ - { TIOCLINUX, KERN_INFO, "TIOCLINUX" }, - { KDGKBMODE, KERN_INFO, "KDGKBMODE" }, - { KDGKBTYPE, KERN_INFO, "KDGKBTYPE" }, - { KDSIGACCEPT, KERN_INFO, "KDSIGACCEPT" }, -}; - -int line_ioctl(struct tty_struct *tty, unsigned int cmd, - unsigned long arg) -{ - int ret; - int i; - - ret = 0; - switch(cmd) { -#ifdef TIOCGETP - case TIOCGETP: - case TIOCSETP: - case TIOCSETN: -#endif -#ifdef TIOCGETC - case TIOCGETC: - case TIOCSETC: -#endif -#ifdef TIOCGLTC - case TIOCGLTC: - case TIOCSLTC: -#endif - /* Note: these are out of date as we now have TCGETS2 etc but this - whole lot should probably go away */ - case TCGETS: - case TCSETSF: - case TCSETSW: - case TCSETS: - case TCGETA: - case TCSETAF: - case TCSETAW: - case TCSETA: - case TCXONC: - case TCFLSH: - case TIOCOUTQ: - case TIOCINQ: - case TIOCGLCKTRMIOS: - case TIOCSLCKTRMIOS: - case TIOCPKT: - case TIOCGSOFTCAR: - case TIOCSSOFTCAR: - return -ENOIOCTLCMD; -#if 0 - case TCwhatever: - /* do something */ - break; -#endif - default: - for (i = 0; i < ARRAY_SIZE(tty_ioctls); i++) - if (cmd == tty_ioctls[i].cmd) - break; - if (i == ARRAY_SIZE(tty_ioctls)) { - printk(KERN_ERR "%s: %s: unknown ioctl: 0x%x\n", - __func__, tty->name, cmd); - } - ret = -ENOIOCTLCMD; - break; - } - return ret; -} - void line_throttle(struct tty_struct *tty) { struct line *line = tty->driver_data; diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h index 5b3d4fb..6c3b252 100644 --- a/arch/um/drivers/line.h +++ b/arch/um/drivers/line.h @@ -69,8 +69,6 @@ extern int line_chars_in_buffer(struct tty_struct *tty); extern void line_flush_buffer(struct tty_struct *tty); 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, unsigned int cmd, - unsigned long arg); extern void line_throttle(struct tty_struct *tty); extern void line_unthrottle(struct tty_struct *tty); diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c index e09801a..a39d53e 100644 --- a/arch/um/drivers/ssl.c +++ b/arch/um/drivers/ssl.c @@ -129,7 +129,6 @@ static const struct tty_operations ssl_ops = { .flush_buffer = line_flush_buffer, .flush_chars = line_flush_chars, .set_termios = line_set_termios, - .ioctl = line_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 7663541..5cefdba 100644 --- a/arch/um/drivers/stdio_console.c +++ b/arch/um/drivers/stdio_console.c @@ -112,7 +112,6 @@ static const struct tty_operations console_ops = { .flush_buffer = line_flush_buffer, .flush_chars = line_flush_chars, .set_termios = line_set_termios, - .ioctl = line_ioctl, .throttle = line_throttle, .unthrottle = line_unthrottle, }; -- 1.7.7.3 |
From: Richard W. <ri...@no...> - 2012-06-04 20:28:00
|
Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/ssl.c | 26 -------------------------- 1 files changed, 0 insertions(+), 26 deletions(-) diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c index a39d53e..cba95d9 100644 --- a/arch/um/drivers/ssl.c +++ b/arch/um/drivers/ssl.c @@ -98,27 +98,6 @@ static int ssl_open(struct tty_struct *tty, struct file *filp) return err; } -#if 0 -static void ssl_flush_buffer(struct tty_struct *tty) -{ - return; -} - -static void ssl_stop(struct tty_struct *tty) -{ - printk(KERN_ERR "Someone should implement ssl_stop\n"); -} - -static void ssl_start(struct tty_struct *tty) -{ - printk(KERN_ERR "Someone should implement ssl_start\n"); -} - -void ssl_hangup(struct tty_struct *tty) -{ -} -#endif - static const struct tty_operations ssl_ops = { .open = ssl_open, .close = line_close, @@ -131,11 +110,6 @@ static const struct tty_operations ssl_ops = { .set_termios = line_set_termios, .throttle = line_throttle, .unthrottle = line_unthrottle, -#if 0 - .stop = ssl_stop, - .start = ssl_start, - .hangup = ssl_hangup, -#endif }; /* Changed by ssl_init and referenced by ssl_exit, which are both serialized -- 1.7.7.3 |
From: Richard W. <ri...@no...> - 2012-06-04 20:28:00
|
this lock is no longer needed. Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/line.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index 95d5e78..555ccfc 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -382,8 +382,6 @@ int setup_one_line(struct line *lines, int n, char *init, struct tty_driver *driver = line->driver->driver; int err = -EINVAL; - mutex_lock(&line->count_lock); - if (line->port.count) { *error_out = "Device is already open"; goto out; @@ -425,7 +423,6 @@ int setup_one_line(struct line *lines, int n, char *init, } } out: - mutex_unlock(&line->count_lock); return err; } @@ -513,7 +510,6 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str, line = &lines[dev]; - mutex_lock(&line->count_lock); if (!line->valid) CONFIG_CHUNK(str, size, n, "none", 1); else { @@ -525,7 +521,6 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str, tty_kref_put(tty); } } - mutex_unlock(&line->count_lock); return n; } @@ -578,7 +573,6 @@ int register_lines(struct line_driver *line_driver, tty_port_init(&lines[i].port); lines[i].port.ops = &line_port_ops; spin_lock_init(&lines[i].lock); - mutex_init(&lines[i].count_lock); lines[i].driver = line_driver; INIT_LIST_HEAD(&lines[i].chan_list); } -- 1.7.7.3 |
From: Richard W. <ri...@no...> - 2012-06-04 20:28:00
|
... use all tty_port helpers Signed-off-by: Richard Weinberger <ri...@no...> --- arch/um/drivers/line.c | 102 +++++++++++++++++---------------------- arch/um/drivers/line.h | 6 ++- arch/um/drivers/ssl.c | 15 ++---- arch/um/drivers/stdio_console.c | 20 ++++---- 4 files changed, 65 insertions(+), 78 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index 131129a..95d5e78 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -296,43 +296,14 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data) return err; } -/* - * Normally, a driver like this can rely mostly on the tty layer - * locking, particularly when it comes to the driver structure. - * However, in this case, mconsole requests can come in "from the - * side", and race with opens and closes. - * - * mconsole config requests will want to be sure the device isn't in - * use, and get_config, open, and close will want a stable - * configuration. The checking and modification of the configuration - * is done under a spinlock. Checking whether the device is in use is - * line->tty->count > 1, also under the spinlock. - * - * line->count serves to decide whether the device should be enabled or - * disabled on the host. If it's equal to 0, then we are doing the - * first open or last close. Otherwise, open and close just return. - */ - -int line_open(struct line *lines, struct tty_struct *tty) +static int line_activate(struct tty_port *port, struct tty_struct *tty) { - struct line *line = &lines[tty->index]; - int err = -ENODEV; - - mutex_lock(&line->count_lock); - if (!line->valid) - goto out_unlock; - - err = 0; - if (line->port.count++) - goto out_unlock; - - BUG_ON(tty->driver_data); - tty->driver_data = line; - tty_port_tty_set(&line->port, tty); + int ret; + struct line *line = tty->driver_data; - err = enable_chan(line); - if (err) /* line_close() will be called by our caller */ - goto out_unlock; + ret = enable_chan(line); + if (ret) + return ret; if (!line->sigio) { chan_enable_winch(line->chan_out, tty); @@ -340,44 +311,60 @@ int line_open(struct line *lines, struct tty_struct *tty) } chan_window_size(line, &tty->winsize.ws_row, - &tty->winsize.ws_col); -out_unlock: - mutex_unlock(&line->count_lock); - return err; + &tty->winsize.ws_col); + + return 0; } -static void unregister_winch(struct tty_struct *tty); +static const struct tty_port_operations line_port_ops = { + .activate = line_activate, +}; -void line_close(struct tty_struct *tty, struct file * filp) +int line_open(struct tty_struct *tty, struct file *filp) { struct line *line = tty->driver_data; - /* - * If line_open fails (and tty->driver_data is never set), - * tty_open will call line_close. So just return in this case. - */ - if (line == NULL) - return; + return tty_port_open(&line->port, tty, filp); +} - /* We ignore the error anyway! */ - flush_buffer(line); +int line_install(struct tty_driver *driver, struct tty_struct *tty, + struct line *line) +{ + int ret; - mutex_lock(&line->count_lock); - BUG_ON(!line->valid); + ret = tty_standard_install(driver, tty); + if (ret) + return ret; + + tty->driver_data = line; - if (--line->port.count) - goto out_unlock; + return 0; +} + +static void unregister_winch(struct tty_struct *tty); - tty_port_tty_set(&line->port, NULL); - tty->driver_data = NULL; +void line_cleanup(struct tty_struct *tty) +{ + struct line *line = tty->driver_data; if (line->sigio) { unregister_winch(tty); line->sigio = 0; } +} -out_unlock: - mutex_unlock(&line->count_lock); +void line_close(struct tty_struct *tty, struct file * filp) +{ + struct line *line = tty->driver_data; + + tty_port_close(&line->port, tty, filp); +} + +void line_hangup(struct tty_struct *tty) +{ + struct line *line = tty->driver_data; + + tty_port_hangup(&line->port); } void close_lines(struct line *lines, int nlines) @@ -589,6 +576,7 @@ int register_lines(struct line_driver *line_driver, for (i = 0; i < nlines; i++) { tty_port_init(&lines[i].port); + lines[i].port.ops = &line_port_ops; spin_lock_init(&lines[i].lock); mutex_init(&lines[i].count_lock); lines[i].driver = line_driver; diff --git a/arch/um/drivers/line.h b/arch/um/drivers/line.h index 6c3b252..ad00f3e 100644 --- a/arch/um/drivers/line.h +++ b/arch/um/drivers/line.h @@ -58,7 +58,11 @@ struct line { }; extern void line_close(struct tty_struct *tty, struct file * filp); -extern int line_open(struct line *lines, struct tty_struct *tty); +extern int line_open(struct tty_struct *tty, struct file *filp); +extern int line_install(struct tty_driver *driver, struct tty_struct *tty, + struct line *line); +extern void line_cleanup(struct tty_struct *tty); +extern void line_hangup(struct tty_struct *tty); extern int line_setup(char **conf, unsigned nlines, char **def, char *init, char *name); extern int line_write(struct tty_struct *tty, const unsigned char *buf, diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c index cba95d9..7e86f00 100644 --- a/arch/um/drivers/ssl.c +++ b/arch/um/drivers/ssl.c @@ -87,19 +87,13 @@ static int ssl_remove(int n, char **error_out) error_out); } -static int ssl_open(struct tty_struct *tty, struct file *filp) +static int ssl_install(struct tty_driver *driver, struct tty_struct *tty) { - int err = line_open(serial_lines, tty); - - if (err) - printk(KERN_ERR "Failed to open serial line %d, err = %d\n", - tty->index, err); - - return err; + return line_install(driver, tty, &serial_lines[tty->index]); } static const struct tty_operations ssl_ops = { - .open = ssl_open, + .open = line_open, .close = line_close, .write = line_write, .put_char = line_put_char, @@ -110,6 +104,9 @@ static const struct tty_operations ssl_ops = { .set_termios = line_set_termios, .throttle = line_throttle, .unthrottle = line_unthrottle, + .install = ssl_install, + .cleanup = line_cleanup, + .hangup = line_hangup, }; /* Changed by ssl_init and referenced by ssl_exit, which are both serialized diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c index 5cefdba..929b99a 100644 --- a/arch/um/drivers/stdio_console.c +++ b/arch/um/drivers/stdio_console.c @@ -89,21 +89,17 @@ static int con_remove(int n, char **error_out) return line_remove(vts, ARRAY_SIZE(vts), n, error_out); } -static int con_open(struct tty_struct *tty, struct file *filp) -{ - int err = line_open(vts, tty); - if (err) - printk(KERN_ERR "Failed to open console %d, err = %d\n", - tty->index, err); - - return err; -} - /* Set in an initcall, checked in an exitcall */ static int con_init_done = 0; +static int con_install(struct tty_driver *driver, struct tty_struct *tty) +{ + return line_install(driver, tty, &vts[tty->index]); +} + static const struct tty_operations console_ops = { - .open = con_open, + .open = line_open, + .install = con_install, .close = line_close, .write = line_write, .put_char = line_put_char, @@ -114,6 +110,8 @@ static const struct tty_operations console_ops = { .set_termios = line_set_termios, .throttle = line_throttle, .unthrottle = line_unthrottle, + .cleanup = line_cleanup, + .hangup = line_hangup, }; static void uml_console_write(struct console *console, const char *string, -- 1.7.7.3 |
From: Jiri S. <js...@su...> - 2012-06-04 20:48:05
|
On 06/04/2012 10:27 PM, Richard Weinberger wrote: > +void line_cleanup(struct tty_struct *tty) > +{ > + struct line *line = tty->driver_data; > > if (line->sigio) { > unregister_winch(tty); > line->sigio = 0; r u sure you want to to do this asynchronously? This should be tty_port_operations->shutdown I suppose... > } > +} Anyway, please split the patch into 2 pieces at least: 1) introducing ->install and moving the setup there. 2) switching to tty port helpers Otherwise it's a hard-to-review mess. thanks, -- js suse labs |
From: Alan C. <al...@lx...> - 2012-06-04 21:14:02
|
> On all other ttys login works but bash dies because of of -EIO. > After vhangup() the tty returns -EIO upon read()/write(). You can't re-open the tty because a process is holding on to it, not closing it and not killable. Fedora shouldn't be holding these devices open this way. The behaviour we have of refusing to reopen them why this is the case is both a) what the spec seems to say b) good security. We can half ignore it on console for the simple reason that you don't "dial in" to the console. I suspect it may be abusable but I've not found a way to do so. > I have no idea what's the root cause of this, there seems to be a lot of black magic > involved. > Alan, do you think the issues are caused by Fedora's broken user space? See what fuser says about open file handles and if that is what is going on. Alan |
From: Richard W. <ri...@no...> - 2012-06-04 23:14:52
Attachments:
signature.asc
|
Am 04.06.2012 23:17, schrieb Alan Cox: >> On all other ttys login works but bash dies because of of -EIO. >> After vhangup() the tty returns -EIO upon read()/write(). > > You can't re-open the tty because a process is holding on to it, not > closing it and not killable. Fedora shouldn't be holding these devices > open this way. The behaviour we have of refusing to reopen them why this > is the case is both a) what the spec seems to say b) good security. Hmm, there seems to be a bug in util-linux's login. login-utils/login.c::init_tty() does: ... /* Kill processes left on this tty */ tcsetattr(0, TCSAFLUSH, &ttt); signal(SIGHUP, SIG_IGN); /* so vhangup() wont kill us */ vhangup(); signal(SIGHUP, SIG_DFL); /* open stdin,stdout,stderr to the tty */ open_tty(cxt->tty_path); /* restore tty modes */ tcsetattr(0, TCSAFLUSH, &tt); ... By calling vhangup() it kills all other programs on the current tty. open_tty() opens the tty again but it's still open because stdin, stdout and stderr belongs to it. If I add: fclose(stdin); fclose(stdout); fclose(stderr); before the call to vhangup() login works like charm. :-) Karel, what do you think? Thanks, //richard |
From: Karel Z. <kz...@re...> - 2012-06-05 10:41:40
|
On Tue, Jun 05, 2012 at 01:14:41AM +0200, Richard Weinberger wrote: > Am 04.06.2012 23:17, schrieb Alan Cox: > >> On all other ttys login works but bash dies because of of -EIO. > >> After vhangup() the tty returns -EIO upon read()/write(). > > > > You can't re-open the tty because a process is holding on to it, not > > closing it and not killable. Fedora shouldn't be holding these devices > > open this way. The behaviour we have of refusing to reopen them why this > > is the case is both a) what the spec seems to say b) good security. > > Hmm, there seems to be a bug in util-linux's login. > login-utils/login.c::init_tty() does: > ... > /* Kill processes left on this tty */ > tcsetattr(0, TCSAFLUSH, &ttt); > > signal(SIGHUP, SIG_IGN); /* so vhangup() wont kill us */ > vhangup(); > signal(SIGHUP, SIG_DFL); > > /* open stdin,stdout,stderr to the tty */ > open_tty(cxt->tty_path); > > /* restore tty modes */ > tcsetattr(0, TCSAFLUSH, &tt); > ... > > By calling vhangup() it kills all other programs on the current tty. > open_tty() opens the tty again but it's still open because stdin, stdout and stderr > belongs to it. open_tty() closes all the old file descriptors after tty open: vhangup(); ... fd = open(tty, O_RDWR | O_NONBLOCK); ... flags = fcntl(fd, F_GETFL); flags &= ~O_NONBLOCK; fcntl(fd, F_SETFL, flags); for (i = 0; i < fd; i++) close(i); for (i = 0; i < 3; i++) if (fd != i) dup2(fd, i); if (fd >= 3) close(fd); we use this for pretty long time (17+ years). > If I add: > fclose(stdin); > fclose(stdout); > fclose(stderr); > before the call to vhangup() login works like charm. :-) > > Karel, what do you think? It's probably no problem to close all the file descriptors before vhangup(), but it would be nice to know why we need this change after 20 years :-) Karel -- Karel Zak <kz...@re...> http://karelzak.blogspot.com |
From: Alan C. <al...@lx...> - 2012-06-05 11:12:06
|
> open_tty() closes all the old file descriptors after tty open: It needs to close them before. > > vhangup(); > ... > fd = open(tty, O_RDWR | O_NONBLOCK); > ... > > flags = fcntl(fd, F_GETFL); > flags &= ~O_NONBLOCK; > fcntl(fd, F_SETFL, flags); > > for (i = 0; i < fd; i++) > close(i); > for (i = 0; i < 3; i++) > if (fd != i) > dup2(fd, i); > if (fd >= 3) > close(fd); > > we use this for pretty long time (17+ years). > > > If I add: > > fclose(stdin); > > fclose(stdout); > > fclose(stderr); > > before the call to vhangup() login works like charm. :-) > > > > Karel, what do you think? > > It's probably no problem to close all the file descriptors before > vhangup(), but it would be nice to know why we need this change after > 20 years :-) Because we want to actually fix the standards (and security) violation that means it has happened to work on the console for 20 years. Actually I'd prefer a clever solution which can spot all the fds are the same process so we can keep compatibility but I've not found a sensible way to do that. Alan |
From: Richard W. <ri...@no...> - 2012-06-05 12:20:59
Attachments:
signature.asc
|
Am 05.06.2012 13:15, schrieb Alan Cox: > Actually I'd prefer a clever solution which can spot all the fds are the > same process so we can keep compatibility but I've not found a sensible > way to do that. Me too. For UML users the current situation is odd. They have to select between: - Having a UML that crashes randomly on some distros. - Applying the TTY fix and break login on every distro that uses util-linux's login. - Applying the TTY fix and patch util-linux too. Thanks, //richard |
From: Karel Z. <kz...@re...> - 2012-06-05 15:17:32
|
On Tue, Jun 05, 2012 at 02:20:38PM +0200, Richard Weinberger wrote: > Am 05.06.2012 13:15, schrieb Alan Cox: > > Actually I'd prefer a clever solution which can spot all the fds are the > > same process so we can keep compatibility but I've not found a sensible > > way to do that. > > Me too. > > For UML users the current situation is odd. > They have to select between: > - Having a UML that crashes randomly on some distros. > - Applying the TTY fix and break login on every distro that uses util-linux's login. > - Applying the TTY fix and patch util-linux too. I'll follow kernel... so if close-before-vhangup is preferred way than I'll modify util-linux login(1). No problem. Karel -- Karel Zak <kz...@re...> http://karelzak.blogspot.com |
From: Boaz H. <bha...@pa...> - 2012-06-06 14:21:18
|
On 06/04/2012 11:27 PM, Richard Weinberger wrote: > This patch set moves the UML console driver to the new TTY port interface. > It does ref counting and uses the tty_port_*-helpers. > Please note, it's not yet UML mconsole safe! > > Anyway, I see some really strange things and I'm not sure whether my patch > is sane or not... > > If I implement tty_operations->hangup() the following happens: > > FC12: > Login on tty0 works fine. > On all other ttys login works but bash dies because of of -EIO. > After vhangup() the tty returns -EIO upon read()/write(). > FC16: > Login broken on all ttys (bash dies with EIO like on FC12). > If I start UML with rootfs read-only login works on tty0. - WTF?! > Debian 6.0: > Login works perfectly fine on all ttys > > Without tty_operations->hangup() the following happens: > FC12: > Login on tty0 works fine. Horray I'm back to my old self. Thanks I'll give it a run and report of any new problems Thanks a million Richard Boaz > mingetty is unable to start on anything else than tty0. > It exits after a few seconds. > FC16: > Unable to start any mingetty (like on FC12 it exits after a few seconds) > With read-only rootfs mingetty starts at least on tty0 and login works. (Again, WTF?) > Debian 6.0 > Login works perfectly fine on all ttys. > > I have no idea what's the root cause of this, there seems to be a lot of black magic > involved. > Alan, do you think the issues are caused by Fedora's broken user space? > How can we fix this? > > Thanks, > //richard (wearing a voodoo priests robe) > > [PATCH 1/6] TTY: um/line, add tty_port > [PATCH 2/6] TTY: um/line, use tty from tty_port > [PATCH 3/6] um: remove line_ioctl() > [PATCH 4/6] um: Remove dead code > [PATCH 5/6] um: fully use tty_port > [PATCH 6/6] um: remove count_lock > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > User-mode-linux-devel mailing list > Use...@li... > https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel |
From: Richard W. <ri...@no...> - 2012-06-06 23:17:36
Attachments:
signature.asc
|
Am 04.06.2012 23:17, schrieb Alan Cox: > We can half ignore it on console for the simple reason that you don't > "dial in" to the console. I suspect it may be abusable but I've not found > a way to do so. > BTW: Can't we add such a mode to tty_port? E.g. tty_port->non_dialin_console. 8) Nobody dials-in on a UML console... Thanks, //richard |
From: Alan C. <al...@li...> - 2012-06-07 09:04:15
|
On Thu, 07 Jun 2012 01:17:24 +0200 Richard Weinberger <ri...@no...> wrote: > Am 04.06.2012 23:17, schrieb Alan Cox: > > We can half ignore it on console for the simple reason that you > > don't "dial in" to the console. I suspect it may be abusable but > > I've not found a way to do so. > > > > BTW: Can't we add such a mode to tty_port? > E.g. tty_port->non_dialin_console. 8) That is basically what you are doing when you omit a hangup method. The security consequences on a multi-user box run deeper however so in the end we need to fix the util-linux code. Alan |
From: Richard W. <ri...@no...> - 2012-06-07 09:06:27
Attachments:
signature.asc
|
Am 07.06.2012 11:19, schrieb Alan Cox: > On Thu, 07 Jun 2012 01:17:24 +0200 > Richard Weinberger <ri...@no...> wrote: > >> Am 04.06.2012 23:17, schrieb Alan Cox: >>> We can half ignore it on console for the simple reason that you >>> don't "dial in" to the console. I suspect it may be abusable but >>> I've not found a way to do so. >>> >> >> BTW: Can't we add such a mode to tty_port? >> E.g. tty_port->non_dialin_console. 8) > > That is basically what you are doing when you omit a hangup method. The > security consequences on a multi-user box run deeper however so in the > end we need to fix the util-linux code. Okay, than I have to find out why getty does not start if I omit ->hangup. Thanks, //richard |
From: Boaz H. <bha...@pa...> - 2012-06-07 07:35:32
|
On 06/06/2012 05:20 PM, Boaz Harrosh wrote: > On 06/04/2012 11:27 PM, Richard Weinberger wrote: > <> > > > Horray I'm back to my old self. Thanks I'll give it a run > and report of any new problems > > Thanks a million Richard > Boaz > OK I've run with these for a few days and they are doing the Job perfectly. Not a single problem. You may add tested-by: Boaz Harrosh <bha...@pa...> on all patches if you'd like. Please push them ASAP to Linus. They fix a real life breakage that's there since v3.3. At least now we are back to the problems that where there for a long time. Please also CC: Stable Tree <st...@ke...> for 3.4 as clearly 3.4 is unusable at all right now. I wish they would be sent to older stables as well, but they won't patch, so it's not worth the effort. >> mingetty is unable to start on anything else than tty0. >> It exits after a few seconds. I suspect this was always so since forever. I don't even know how to configure such a thing. I know that the console I run vmlinux from becomes tty0, until I halt (or crash). How do you open more tty(s) and attach them to console windows? I just use ssh for that. (The setup of tty(s) for UML on Fedora is always such a pain and it changed 4 times since FC10, I always have to mount the image, figure out how to make tty1 a tty0, and delete all the other tty(s). perhaps I'm just clueless. Is there an easier way? Is there a way to tell UML to make tty0 be a tty1? Because in fedora tty0 is not defined.) But please do not delay these patches because of old problems. These can be solved later. Thanks very much for fixing this, it's a life saver Boaz |
From: Boaz H. <bha...@pa...> - 2012-06-07 07:43:33
|
On 06/07/2012 10:35 AM, Boaz Harrosh wrote: <> > > Please also CC: Stable Tree <st...@ke...> for 3.4 > as clearly 3.4 is unusable at all right now. > I forgot to ask: Please CC: me on these patches I would like to be notified of their advancements into the different trees. Thanks Boaz |
From: Richard W. <ri...@no...> - 2012-06-07 08:45:40
Attachments:
signature.asc
|
Am 07.06.2012 09:35, schrieb Boaz Harrosh: > > OK I've run with these for a few days and they are doing > the Job perfectly. Not a single problem. Good to know. > You may add tested-by: Boaz Harrosh <bha...@pa...> > on all patches if you'd like. > > Please push them ASAP to Linus. They fix a real life breakage > that's there since v3.3. At least now we are back to > the problems that where there for a long time. > > Please also CC: Stable Tree <st...@ke...> for 3.4 > as clearly 3.4 is unusable at all right now. We cannot push this patch to Linus or -stable. The problem is that will break other things. E.g. login on non-tty0 terminals will break if the distro uses util-linux's login. > I wish they would be sent to older stables as well, but > they won't patch, so it's not worth the effort. > >>> mingetty is unable to start on anything else than tty0. >>> It exits after a few seconds. > > > I suspect this was always so since forever. I don't even know > how to configure such a thing. I know that the console I run > vmlinux from becomes tty0, until I halt (or crash). How do > you open more tty(s) and attach them to console windows? > I just use ssh for that. con=pts is very nice. UML allocates a pts for each tty on the *host* side. Then you can attach to this tty using screen. See: http://user-mode-linux.sourceforge.net/old/input.html > (The setup of tty(s) for UML on Fedora is always such a pain > and it changed 4 times since FC10, I always have to mount > the image, figure out how to make tty1 a tty0, and delete > all the other tty(s). perhaps I'm just clueless. Is there an > easier way? Is there a way to tell UML to make tty0 be a tty1? > Because in fedora tty0 is not defined.) AFAIK no. But you can tell Fedora to use tty0. Even with systemd this is possible. :-) > But please do not delay these patches because of old problems. > These can be solved later. Breaking existing applications is a no-go, sorry. Thanks, //richard |
From: Alan C. <al...@li...> - 2012-06-07 09:06:52
|
> Breaking existing applications is a no-go, sorry. Being insecure should also be a no-no. Not sure what Jiri thinks but for the moment I think we need to push it with a module option as to whether hangup on console is enabled or not. I don't want to just break the existing user space, but leaving other vendors systems insecure just to cover Fedora's backside is also not entirely fair either. Alan |
From: Boaz H. <bha...@pa...> - 2012-06-07 10:15:11
|
On 06/07/2012 12:22 PM, Alan Cox wrote: > On 06/07/2012 11:45 AM, Richard Weinberger wrote: >> >> We cannot push this patch to Linus or -stable. >> The problem is that will break other things. >> E.g. login on non-tty0 terminals will break if the distro uses >> util-linux's login. >> I don't understand. Current code does not work at all even for tty0. as well as ttyX. Since 3-4 Kernels ago. I've been running with your patch for a long time. I really don't get it. You have not broken anything new. Only not fixed all of the problems. Current code does not work for "non-tty0 terminals" as well right? <> >> Breaking existing applications is a no-go, sorry. > Being insecure should also be a no-no. > > Not sure what Jiri thinks but for the moment I think we need to push it > with a module option as to whether hangup on console is enabled or not. > > I don't want to just break the existing user space, but leaving other > vendors systems insecure just to cover Fedora's backside is also not > entirely fair either. I don't see Alan's comment at all. This is not a regression it was always like that. Ever since Fedora was working on UML, But these fixes are real live regression crashes. And I don't see the all "leaving other vendors systems insecure". It just a freaking UML tty. You need to be root 5 times before you have access to all these, and it's only the UML that's compromised not the "all system" And surely the current plain tty0 crash is much less secure then this thing. > Thanks, > //richard > Please let us work, I don't see the point of leaving something terminally broken, ever. Thanks Boaz |
From: Richard W. <ri...@no...> - 2012-06-07 10:19:38
Attachments:
signature.asc
|
Am 07.06.2012 12:14, schrieb Boaz Harrosh: > On 06/07/2012 12:22 PM, Alan Cox wrote: >> On 06/07/2012 11:45 AM, Richard Weinberger wrote: > >>> >>> We cannot push this patch to Linus or -stable. >>> The problem is that will break other things. >>> E.g. login on non-tty0 terminals will break if the distro uses >>> util-linux's login. >>> > > > I don't understand. Current code does not work at all even for > tty0. as well as ttyX. Since 3-4 Kernels ago. I've been running with > your patch for a long time. Depends on your userspace. On my setups it's very hard to trigger the bug. > I really don't get it. You have not broken anything new. Only > not fixed all of the problems. Current code does not work for "non-tty0 > terminals" as well right? No, it works fine. > I don't see Alan's comment at all. This is not a regression it was always > like that. Ever since Fedora was working on UML, But these fixes are real > live regression crashes. > > And I don't see the all "leaving other vendors systems insecure". It just > a freaking UML tty. You need to be root 5 times before you have access > to all these, and it's only the UML that's compromised not the "all system" > And surely the current plain tty0 crash is much less secure then this thing. The "TTY problem" is not UML specific. Thanks, //richard |
From: Boaz H. <bha...@pa...> - 2012-06-07 10:35:40
|
On 06/07/2012 01:19 PM, Richard Weinberger wrote: > > No, it works fine. > OK Sorry I missed that part. So you are saying that these patches fix Fedora, but completely break other systems which now work? I saw in your cover letter that Debian was fine with or without ->hangup() so I assumed all other system are more like Debian. >> I don't see Alan's comment at all. This is not a regression it was always >> like that. Ever since Fedora was working on UML, But these fixes are real >> live regression crashes. >> >> And I don't see the all "leaving other vendors systems insecure". It just >> a freaking UML tty. You need to be root 5 times before you have access >> to all these, and it's only the UML that's compromised not the "all system" >> And surely the current plain tty0 crash is much less secure then this thing. > > The "TTY problem" is not UML specific. > Exactly my point, so it is not anything your UM-only patches can do anything about it. right? > Thanks, > //richard > I guess I will have to carry these longer. Can I fix my FC12 and FC15 so they don't crash with mainline? Thanks Boaz |