From d79a580936396bbcd2f4fae2c6215f9cf81e3c0d Mon Sep 17 00:00:00 2001 From: Jeff Dike Date: Sat, 10 Feb 2007 01:43:52 -0800 Subject: [PATCH] [PATCH] uml: console locking fixes Clean up the console driver locking. There are various problems here, including sleeping under a spinlock and spinlock recursion, some of which are fixed here. This patch deals with the locking involved with opens and closes. The problem is that an mconsole request to change a console's configuration can race with an open. Changing a configuration should only be done when a console isn't opened. Also, an open must be looking at a stable configuration. In addition, a get configuration request must observe the same locking since it must also see a stable configuration. With the old locking, it was possible for this to hang indefinitely in some cases because open would block for a long time waiting for a connection from the host while holding the lock needed by the mconsole request. As explained in the long comment, this is fixed by adding a spinlock for the use count and configuration and a mutex for the actual open and close. Signed-off-by: Jeff Dike Cc: Paolo 'Blaisorblade' Giarrusso Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/line.c | 186 +++++++++++++++++++++----------- arch/um/drivers/stdio_console.c | 6 +- arch/um/include/line.h | 14 ++- 3 files changed, 134 insertions(+), 72 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index 83301e1ef67c..799fca3644e9 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -191,7 +191,6 @@ void line_flush_buffer(struct tty_struct *tty) /*XXX: copied from line_write, verify if it is correct!*/ if(tty->stopped) return; - //return 0; spin_lock_irqsave(&line->lock, flags); err = flush_buffer(line); @@ -421,42 +420,84 @@ 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. + * + * The problem comes from line_setup not wanting to sleep if + * the device is open or being opened. This can happen because the + * first opener of a device is responsible for setting it up on the + * host, and that can sleep. The open of a port device will sleep + * until someone telnets to it. + * + * The obvious solution of putting everything under a mutex fails + * because then trying (and failing) to change the configuration of an + * open(ing) device will block until the open finishes. The right + * thing to happen is for it to fail immediately. + * + * We can put the opening (and closing) of the host device under a + * separate lock, but that has to be taken before the count lock is + * released. Otherwise, you open a window in which another open can + * come through and assume that the host side is opened and working. + * + * So, if the tty count is one, open will take the open mutex + * inside the count lock. Otherwise, it just returns. This will sleep + * if the last close is pending, and will block a setup or get_config, + * but that should not last long. + * + * So, what we end up with is that open and close take the count lock. + * If the first open or last close are happening, then the open mutex + * is taken inside the count lock and the host opening or closing is done. + * + * setup and get_config only take the count lock. setup modifies the + * device configuration only if the open count is zero. Arbitrarily + * long blocking of setup doesn't happen because something would have to be + * waiting for an open to happen. However, a second open with + * tty->count == 1 can't happen, and a close can't happen until the open + * had finished. + * + * We can't maintain our own count here because the tty layer doesn't + * match opens and closes. It will call close if an open failed, and + * a tty hangup will result in excess closes. So, we rely on + * tty->count instead. It is one on both the first open and last close. + */ + int line_open(struct line *lines, struct tty_struct *tty) { - struct line *line; + struct line *line = &lines[tty->index]; int err = -ENODEV; - line = &lines[tty->index]; - tty->driver_data = line; + spin_lock(&line->count_lock); + if(!line->valid) + goto out_unlock; + + err = 0; + if(tty->count > 1) + goto out_unlock; - /* The IRQ which takes this lock is not yet enabled and won't be run - * before the end, so we don't need to use spin_lock_irq.*/ - spin_lock(&line->lock); + mutex_lock(&line->open_mutex); + spin_unlock(&line->count_lock); tty->driver_data = line; line->tty = tty; - if(!line->valid) - goto out; - if(tty->count == 1){ - /* Here the device is opened, if necessary, and interrupt - * is registered. - */ - enable_chan(line); - INIT_DELAYED_WORK(&line->task, line_timer_cb); - - if(!line->sigio){ - chan_enable_winch(&line->chan_list, tty); - line->sigio = 1; - } + enable_chan(line); + INIT_DELAYED_WORK(&line->task, line_timer_cb); - chan_window_size(&line->chan_list, &tty->winsize.ws_row, - &tty->winsize.ws_col); + if(!line->sigio){ + chan_enable_winch(&line->chan_list, tty); + line->sigio = 1; } - err = 0; -out: - spin_unlock(&line->lock); + chan_window_size(&line->chan_list, &tty->winsize.ws_row, + &tty->winsize.ws_col); + + mutex_unlock(&line->open_mutex); + return err; + +out_unlock: + spin_unlock(&line->count_lock); return err; } @@ -466,25 +507,38 @@ void line_close(struct tty_struct *tty, struct file * filp) { struct line *line = tty->driver_data; - /* XXX: I assume this should be called in process context, not with - * interrupts disabled! - */ - spin_lock_irq(&line->lock); + /* 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; /* We ignore the error anyway! */ flush_buffer(line); - if(tty->count == 1){ - line->tty = NULL; - tty->driver_data = NULL; + spin_lock(&line->count_lock); + if(!line->valid) + goto out_unlock; + + if(tty->count > 1) + goto out_unlock; - if(line->sigio){ - unregister_winch(tty); - line->sigio = 0; - } + mutex_lock(&line->open_mutex); + spin_unlock(&line->count_lock); + + line->tty = NULL; + tty->driver_data = NULL; + + if(line->sigio){ + unregister_winch(tty); + line->sigio = 0; } - spin_unlock_irq(&line->lock); + mutex_unlock(&line->open_mutex); + return; + +out_unlock: + spin_unlock(&line->count_lock); } void close_lines(struct line *lines, int nlines) @@ -495,6 +549,30 @@ void close_lines(struct line *lines, int nlines) close_chan(&lines[i].chan_list, 0); } +static void setup_one_line(struct line *lines, int n, char *init, int init_prio) +{ + struct line *line = &lines[n]; + + spin_lock(&line->count_lock); + + if(line->tty != NULL){ + printk("line_setup - device %d is open\n", n); + goto out; + } + + if (line->init_pri <= init_prio){ + line->init_pri = init_prio; + if (!strcmp(init, "none")) + line->valid = 0; + else { + line->init_str = init; + line->valid = 1; + } + } +out: + spin_unlock(&line->count_lock); +} + /* Common setup code for both startup command line and mconsole initialization. * @lines contains the array (of size @num) to modify; * @init is the setup string; @@ -526,32 +604,11 @@ int line_setup(struct line *lines, unsigned int num, char *init) n, num - 1); return 0; } - else if (n >= 0){ - if (lines[n].tty != NULL) { - printk("line_setup - device %d is open\n", n); - return 0; - } - if (lines[n].init_pri <= INIT_ONE){ - lines[n].init_pri = INIT_ONE; - if (!strcmp(init, "none")) - lines[n].valid = 0; - else { - lines[n].init_str = init; - lines[n].valid = 1; - } - } - } + else if (n >= 0) + setup_one_line(lines, n, init, INIT_ONE); else { - for(i = 0; i < num; i++){ - if(lines[i].init_pri <= INIT_ALL){ - lines[i].init_pri = INIT_ALL; - if(!strcmp(init, "none")) lines[i].valid = 0; - else { - lines[i].init_str = init; - lines[i].valid = 1; - } - } - } + for(i = 0; i < num; i++) + setup_one_line(lines, i, init, INIT_ALL); } return n == -1 ? num : n; } @@ -602,13 +659,13 @@ int line_get_config(char *name, struct line *lines, unsigned int num, char *str, line = &lines[dev]; - spin_lock(&line->lock); + spin_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->chan_list, str, size, error_out); - spin_unlock(&line->lock); + spin_unlock(&line->count_lock); return n; } @@ -688,6 +745,7 @@ void lines_init(struct line *lines, int nlines, struct chan_opts *opts) for(i = 0; i < nlines; i++){ line = &lines[i]; INIT_LIST_HEAD(&line->chan_list); + mutex_init(&line->open_mutex); if(line->init_str == NULL) continue; diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c index 7a4897e27f42..9b2dd0b8a43b 100644 --- a/arch/um/drivers/stdio_console.c +++ b/arch/um/drivers/stdio_console.c @@ -83,9 +83,9 @@ static struct lines console_lines = LINES_INIT(MAX_TTYS); /* The array is initialized by line_init, which is an initcall. The * individual elements are protected by individual semaphores. */ -struct line vts[MAX_TTYS] = { LINE_INIT(CONFIG_CON_ZERO_CHAN, &driver), - [ 1 ... MAX_TTYS - 1 ] = - LINE_INIT(CONFIG_CON_CHAN, &driver) }; +static struct line vts[MAX_TTYS] = { LINE_INIT(CONFIG_CON_ZERO_CHAN, &driver), + [ 1 ... MAX_TTYS - 1 ] = + LINE_INIT(CONFIG_CON_CHAN, &driver) }; static int con_config(char *str) { diff --git a/arch/um/include/line.h b/arch/um/include/line.h index 5f232ae89fbb..b79643eeee08 100644 --- a/arch/um/include/line.h +++ b/arch/um/include/line.h @@ -11,6 +11,7 @@ #include "linux/tty.h" #include "linux/interrupt.h" #include "linux/spinlock.h" +#include "linux/mutex.h" #include "chan_user.h" #include "mconsole_kern.h" @@ -32,15 +33,17 @@ struct line_driver { struct line { struct tty_struct *tty; + spinlock_t count_lock; + int valid; + + struct mutex open_mutex; char *init_str; int init_pri; struct list_head chan_list; - int valid; - int count; - int throttled; + /*This lock is actually, mostly, local to*/ spinlock_t lock; - + int throttled; /* Yes, this is a real circular buffer. * XXX: And this should become a struct kfifo! * @@ -57,7 +60,8 @@ struct line { }; #define LINE_INIT(str, d) \ - { .init_str = str, \ + { .count_lock = SPIN_LOCK_UNLOCKED, \ + .init_str = str, \ .init_pri = INIT_STATIC, \ .valid = 1, \ .lock = SPIN_LOCK_UNLOCKED, \ -- 2.20.1