From b40997b872cdb70140f127af6069f00a86b6cf81 Mon Sep 17 00:00:00 2001 From: Jonathan Neuschäfer Date: Wed, 14 Sep 2011 16:21:20 -0700 Subject: um: drivers/xterm.c: fix a file descriptor leak I could use out_close1, but that seems to be the code path to close the fd returned by os_create_unix_socket, and using it to close the fd returned by mkstemp might lead to some confusion, so I don't do it. Signed-off-by: Jonathan Neuschäfer Signed-off-by: Richard Weinberger Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/xterm.c | 1 + 1 file changed, 1 insertion(+) (limited to 'arch/um/drivers') diff --git a/arch/um/drivers/xterm.c b/arch/um/drivers/xterm.c index 8ac7146c237f..2e1de5728604 100644 --- a/arch/um/drivers/xterm.c +++ b/arch/um/drivers/xterm.c @@ -123,6 +123,7 @@ static int xterm_open(int input, int output, int primary, void *d, err = -errno; printk(UM_KERN_ERR "xterm_open : unlink failed, errno = %d\n", errno); + close(fd); return err; } close(fd); -- cgit v1.2.3-55-g7522 From f71f94845e0126884eca8ce57a92e30b189c8e71 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 14 Sep 2011 16:21:25 -0700 Subject: um: fix oopsable race in line_close() tty->count is decremented only after ->close() had been called and several tasks can hit it in parallel. As the result, using tty->count to check if you are the last one is broken. We end up leaving line->tty not reset to NULL and the next IRQ on that sucker will blow up trying to dereference pointers from kfree'd struct tty. Fix is obvious: we need to use a counter of our own. Signed-off-by: Al Viro Signed-off-by: Richard Weinberger Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/line.c | 25 ++++++++++++------------- arch/um/include/shared/line.h | 1 + 2 files changed, 13 insertions(+), 13 deletions(-) (limited to 'arch/um/drivers') diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index d51c404239a8..c5bff1ddeabc 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -399,8 +399,8 @@ int line_setup_irq(int fd, int input, int output, struct line *line, void *data) * is done under a spinlock. Checking whether the device is in use is * line->tty->count > 1, also under the spinlock. * - * tty->count serves to decide whether the device should be enabled or - * disabled on the host. If it's equal to 1, then we are doing the + * 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. */ @@ -414,16 +414,16 @@ int line_open(struct line *lines, struct tty_struct *tty) goto out_unlock; err = 0; - if (tty->count > 1) + if (line->count++) goto out_unlock; - spin_unlock(&line->count_lock); - + BUG_ON(tty->driver_data); tty->driver_data = line; line->tty = tty; + spin_unlock(&line->count_lock); err = enable_chan(line); - if (err) + if (err) /* line_close() will be called by our caller */ return err; INIT_DELAYED_WORK(&line->task, line_timer_cb); @@ -436,7 +436,7 @@ int line_open(struct line *lines, struct tty_struct *tty) chan_window_size(&line->chan_list, &tty->winsize.ws_row, &tty->winsize.ws_col); - return err; + return 0; out_unlock: spin_unlock(&line->count_lock); @@ -460,17 +460,16 @@ void line_close(struct tty_struct *tty, struct file * filp) flush_buffer(line); spin_lock(&line->count_lock); - if (!line->valid) - goto out_unlock; + BUG_ON(!line->valid); - if (tty->count > 1) + if (--line->count) goto out_unlock; - spin_unlock(&line->count_lock); - line->tty = NULL; tty->driver_data = NULL; + spin_unlock(&line->count_lock); + if (line->sigio) { unregister_winch(tty); line->sigio = 0; @@ -498,7 +497,7 @@ static int setup_one_line(struct line *lines, int n, char *init, int init_prio, spin_lock(&line->count_lock); - if (line->tty != NULL) { + if (line->count) { *error_out = "Device is already open"; goto out; } diff --git a/arch/um/include/shared/line.h b/arch/um/include/shared/line.h index 72f4f25af247..63df3ca02ac2 100644 --- a/arch/um/include/shared/line.h +++ b/arch/um/include/shared/line.h @@ -33,6 +33,7 @@ struct line_driver { struct line { struct tty_struct *tty; spinlock_t count_lock; + unsigned long count; int valid; char *init_str; -- cgit v1.2.3-55-g7522 From 45cd5e2d4e632f55af1d6131f33b554c98f8b929 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 14 Sep 2011 16:21:28 -0700 Subject: um: winch_interrupt() can happen inside of free_winch() ... so set winch->fd to -1 before doing free_irq(), to avoid having winch_interrupt() come from/during the latter and attempt to do reactivate_fd() on something that's already gone. Signed-off-by: Al Viro Signed-off-by: Richard Weinberger Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/line.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'arch/um/drivers') diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index c5bff1ddeabc..91bf18941ca4 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -725,6 +725,8 @@ struct winch { static void free_winch(struct winch *winch, int free_irq_ok) { + int fd = winch->fd; + winch->fd = -1; if (free_irq_ok) free_irq(WINCH_IRQ, winch); @@ -732,8 +734,8 @@ static void free_winch(struct winch *winch, int free_irq_ok) if (winch->pid != -1) os_kill_process(winch->pid, 1); - if (winch->fd != -1) - os_close_file(winch->fd); + if (fd != -1) + os_close_file(fd); if (winch->stack != 0) free_stack(winch->stack, 0); kfree(winch); -- cgit v1.2.3-55-g7522 From 7cf3cf21aac7d75d27e8e7cd039bd33d19fb300d Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 14 Sep 2011 16:21:31 -0700 Subject: um: fix free_winch() mess while not doing free_irq() from irq handler is commendable, kfree() on the data passed to said handler before free_irq() is Not Good(tm). Freeing the stack it's being run on is also not nice... Solution: delay actually freeing stuff. Signed-off-by: Al Viro Signed-off-by: Richard Weinberger Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/line.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) (limited to 'arch/um/drivers') diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index 91bf18941ca4..364c8a15c4c3 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -721,43 +721,53 @@ struct winch { int pid; struct tty_struct *tty; unsigned long stack; + struct work_struct work; }; -static void free_winch(struct winch *winch, int free_irq_ok) +static void __free_winch(struct work_struct *work) { - int fd = winch->fd; - winch->fd = -1; - if (free_irq_ok) - free_irq(WINCH_IRQ, winch); - - list_del(&winch->list); + struct winch *winch = container_of(work, struct winch, work); + free_irq(WINCH_IRQ, winch); if (winch->pid != -1) os_kill_process(winch->pid, 1); - if (fd != -1) - os_close_file(fd); if (winch->stack != 0) free_stack(winch->stack, 0); kfree(winch); } +static void free_winch(struct winch *winch) +{ + int fd = winch->fd; + winch->fd = -1; + if (fd != -1) + os_close_file(fd); + list_del(&winch->list); + __free_winch(&winch->work); +} + static irqreturn_t winch_interrupt(int irq, void *data) { struct winch *winch = data; struct tty_struct *tty; struct line *line; + int fd = winch->fd; int err; char c; - if (winch->fd != -1) { - err = generic_read(winch->fd, &c, NULL); + if (fd != -1) { + err = generic_read(fd, &c, NULL); if (err < 0) { if (err != -EAGAIN) { + winch->fd = -1; + list_del(&winch->list); + os_close_file(fd); printk(KERN_ERR "winch_interrupt : " "read failed, errno = %d\n", -err); printk(KERN_ERR "fd %d is losing SIGWINCH " "support\n", winch->tty_fd); - free_winch(winch, 0); + INIT_WORK(&winch->work, __free_winch); + schedule_work(&winch->work); return IRQ_HANDLED; } goto out; @@ -829,7 +839,7 @@ static void unregister_winch(struct tty_struct *tty) list_for_each_safe(ele, next, &winch_handlers) { winch = list_entry(ele, struct winch, list); if (winch->tty == tty) { - free_winch(winch, 1); + free_winch(winch); break; } } @@ -845,7 +855,7 @@ static void winch_cleanup(void) list_for_each_safe(ele, next, &winch_handlers) { winch = list_entry(ele, struct winch, list); - free_winch(winch, 1); + free_winch(winch); } spin_unlock(&winch_handler_lock); -- cgit v1.2.3-55-g7522