From: Benjamin Herrenschmidt Here's a new version of the patch, against 2.6.1 bk. I went more in depth into some of the calls in vt_ioctl and I think fixed a few more races along with a possible false-positive. I added the check for oops in progress too. There are matching bits that have to go to fbdev as well, they'll be part of the fbdev merge. --- drivers/char/selection.c | 4 + drivers/char/tty_io.c | 7 +- drivers/char/vt.c | 157 +++++++++++++++++++++++++++++++++-------------- drivers/char/vt_ioctl.c | 61 +++++++++++++----- include/linux/console.h | 8 ++ kernel/power/console.c | 16 ++-- kernel/printk.c | 20 +++++ 7 files changed, 205 insertions(+), 68 deletions(-) diff -puN drivers/char/selection.c~vt-locking-fixes-2 drivers/char/selection.c --- 25/drivers/char/selection.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/drivers/char/selection.c 2004-01-10 21:31:27.000000000 -0800 @@ -24,6 +24,7 @@ #include #include #include +#include #ifndef MIN #define MIN(a,b) ((a) < (b) ? (a) : (b)) @@ -290,7 +291,10 @@ int paste_selection(struct tty_struct *t int pasted = 0, count; DECLARE_WAITQUEUE(wait, current); + acquire_console_sem(); poke_blanked_console(); + release_console_sem(); + add_wait_queue(&vt->paste_wait, &wait); while (sel_buffer && sel_buffer_lth > pasted) { set_current_state(TASK_INTERRUPTIBLE); diff -puN drivers/char/tty_io.c~vt-locking-fixes-2 drivers/char/tty_io.c --- 25/drivers/char/tty_io.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/drivers/char/tty_io.c 2004-01-10 21:31:27.000000000 -0800 @@ -1484,7 +1484,12 @@ static int tiocswinsz(struct tty_struct #ifdef CONFIG_VT if (tty->driver->type == TTY_DRIVER_TYPE_CONSOLE) { unsigned int currcons = tty->index; - if (vc_resize(currcons, tmp_ws.ws_col, tmp_ws.ws_row)) + int rc; + + acquire_console_sem(); + rc = vc_resize(currcons, tmp_ws.ws_col, tmp_ws.ws_row); + release_console_sem(); + if (rc) return -ENXIO; } #endif diff -puN drivers/char/vt.c~vt-locking-fixes-2 drivers/char/vt.c --- 25/drivers/char/vt.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/drivers/char/vt.c 2004-01-10 21:31:27.000000000 -0800 @@ -148,7 +148,6 @@ static const struct consw *con_driver_ma static int con_open(struct tty_struct *, struct file *); static void vc_init(unsigned int console, unsigned int rows, unsigned int cols, int do_clear); -static void blank_screen(unsigned long dummy); static void gotoxy(int currcons, int new_x, int new_y); static void save_cur(int currcons); static void reset_terminal(int currcons, int do_clear); @@ -156,8 +155,8 @@ static void con_flush_chars(struct tty_s static void set_vesa_blanking(unsigned long arg); static void set_cursor(int currcons); static void hide_cursor(int currcons); -static void unblank_screen_t(unsigned long dummy); static void console_callback(void *ignored); +static void blank_screen_t(unsigned long dummy); static int printable; /* Is console ready for printing? */ @@ -214,6 +213,13 @@ static int scrollback_delta; int (*console_blank_hook)(int); static struct timer_list console_timer; +static int blank_state; +static int blank_timer_expired; +enum { + blank_off = 0, + blank_normal_wait, + blank_vesa_wait, +}; /* * Low-Level Functions @@ -337,6 +343,8 @@ static void do_update_region(int currcon void update_region(int currcons, unsigned long start, int count) { + WARN_CONSOLE_UNLOCKED(); + if (DO_UPDATE) { hide_cursor(currcons); do_update_region(currcons, start, count); @@ -400,6 +408,8 @@ void invert_screen(int currcons, int off { unsigned short *p; + WARN_CONSOLE_UNLOCKED(); + count /= 2; p = screenpos(currcons, offset, viewed); if (sw->con_invert_region) @@ -445,6 +455,8 @@ void complement_pos(int currcons, int of static unsigned short old; static unsigned short oldx, oldy; + WARN_CONSOLE_UNLOCKED(); + if (p) { scr_writew(old, p); if (DO_UPDATE) @@ -564,6 +576,8 @@ static void set_cursor(int currcons) static void set_origin(int currcons) { + WARN_CONSOLE_UNLOCKED(); + if (!IS_VISIBLE || !sw->con_set_origin || !sw->con_set_origin(vc_cons[currcons].d)) @@ -575,6 +589,8 @@ static void set_origin(int currcons) static inline void save_screen(int currcons) { + WARN_CONSOLE_UNLOCKED(); + if (sw->con_save_screen) sw->con_save_screen(vc_cons[currcons].d); } @@ -588,6 +604,8 @@ void redraw_screen(int new_console, int int redraw = 1; int currcons, old_console; + WARN_CONSOLE_UNLOCKED(); + if (!vc_cons_allocated(new_console)) { /* strange ... */ /* printk("redraw_screen: tty %d not allocated ??\n", new_console+1); */ @@ -665,6 +683,8 @@ static void visual_init(int currcons, in int vc_allocate(unsigned int currcons) /* return 0 on success */ { + WARN_CONSOLE_UNLOCKED(); + if (currcons >= MAX_NR_CONSOLES) return -ENXIO; if (!vc_cons[currcons].d) { @@ -731,6 +751,8 @@ int vc_resize(int currcons, unsigned int unsigned int new_cols, new_rows, new_row_size, new_screen_size; unsigned short *newscreen; + WARN_CONSOLE_UNLOCKED(); + if (!vc_cons_allocated(currcons)) return -ENXIO; @@ -817,7 +839,8 @@ int vc_resize(int currcons, unsigned int void vc_disallocate(unsigned int currcons) { - acquire_console_sem(); + WARN_CONSOLE_UNLOCKED(); + if (vc_cons_allocated(currcons)) { sw->con_deinit(vc_cons[currcons].d); if (kmalloced) @@ -826,7 +849,6 @@ void vc_disallocate(unsigned int currcon kfree(vc_cons[currcons].d); vc_cons[currcons].d = NULL; } - release_console_sem(); } /* @@ -2081,6 +2103,10 @@ static void console_callback(void *ignor sw->con_scrolldelta(vc_cons[currcons].d, scrollback_delta); scrollback_delta = 0; } + if (blank_timer_expired) { + do_blank_screen(0); + blank_timer_expired = 0; + } release_console_sem(); } @@ -2414,7 +2440,9 @@ static int con_open(struct tty_struct *t currcons = tty->index; + acquire_console_sem(); i = vc_allocate(currcons); + release_console_sem(); if (i) return i; @@ -2480,16 +2508,20 @@ static int __init con_init(void) const char *display_desc = NULL; unsigned int currcons = 0; + acquire_console_sem(); + if (conswitchp) display_desc = conswitchp->con_startup(); if (!display_desc) { fg_console = 0; + release_console_sem(); return 0; } init_timer(&console_timer); - console_timer.function = blank_screen; + console_timer.function = blank_screen_t; if (blankinterval) { + blank_state = blank_normal_wait; mod_timer(&console_timer, jiffies + blankinterval); } @@ -2520,6 +2552,8 @@ static int __init con_init(void) printable = 1; printk("\n"); + release_console_sem(); + #ifdef CONFIG_VT_CONSOLE register_console(&vt_console_driver); #endif @@ -2600,8 +2634,13 @@ void take_over_console(const struct cons int i, j = -1; const char *desc; + acquire_console_sem(); + desc = csw->con_startup(); - if (!desc) return; + if (!desc) { + release_console_sem(); + return; + } if (deflt) conswitchp = csw; @@ -2641,6 +2680,8 @@ void take_over_console(const struct cons desc, vc_cons[j].d->vc_cols, vc_cons[j].d->vc_rows); else printk("to %s\n", desc); + + release_console_sem(); } void give_up_console(const struct consw *csw) @@ -2689,23 +2730,24 @@ static void vesa_powerdown(void) } } -/* - * This is a timer handler - */ -static void vesa_powerdown_screen(unsigned long dummy) -{ - console_timer.function = unblank_screen_t; - - vesa_powerdown(); -} - -static void timer_do_blank_screen(int entering_gfx, int from_timer_handler) +void do_blank_screen(int entering_gfx) { int currcons = fg_console; int i; - if (console_blanked) + WARN_CONSOLE_UNLOCKED(); + + if (console_blanked) { + if (blank_state == blank_vesa_wait) { + blank_state = blank_off; + vesa_powerdown(); + + } return; + } + if (blank_state != blank_normal_wait) + return; + blank_state = blank_off; /* entering graphics mode? */ if (entering_gfx) { @@ -2724,9 +2766,8 @@ static void timer_do_blank_screen(int en } hide_cursor(currcons); - if (!from_timer_handler) - del_timer_sync(&console_timer); - console_timer.function = unblank_screen_t; + del_timer_sync(&console_timer); + blank_timer_expired = 0; save_screen(currcons); /* In case we need to reset origin, blanking hook returns 1 */ @@ -2739,7 +2780,7 @@ static void timer_do_blank_screen(int en return; if (vesa_off_interval) { - console_timer.function = vesa_powerdown_screen; + blank_state = blank_vesa_wait, mod_timer(&console_timer, jiffies + vesa_off_interval); } @@ -2747,18 +2788,6 @@ static void timer_do_blank_screen(int en sw->con_blank(vc_cons[currcons].d, vesa_blank_mode + 1); } -void do_blank_screen(int entering_gfx) -{ - timer_do_blank_screen(entering_gfx, 0); -} - -/* - * This is a timer handler - */ -static void unblank_screen_t(unsigned long dummy) -{ - unblank_screen(); -} /* * Called by timer as well as from vt_console_driver @@ -2767,6 +2796,8 @@ void unblank_screen(void) { int currcons; + WARN_CONSOLE_UNLOCKED(); + ignore_poke = 0; if (!console_blanked) return; @@ -2779,9 +2810,9 @@ void unblank_screen(void) if (vcmode != KD_TEXT) return; /* but leave console_blanked != 0 */ - console_timer.function = blank_screen; if (blankinterval) { mod_timer(&console_timer, jiffies + blankinterval); + blank_state = blank_normal_wait; } console_blanked = 0; @@ -2795,23 +2826,33 @@ void unblank_screen(void) } /* - * This is both a user-level callable and a timer handler + * We defer the timer blanking to work queue so it can take the console semaphore + * (console operations can still happen at irq time, but only from printk which + * has the console semaphore. Not perfect yet, but better than no locking */ -static void blank_screen(unsigned long dummy) +static void blank_screen_t(unsigned long dummy) { - timer_do_blank_screen(0, 1); + blank_timer_expired = 1; + schedule_work(&console_work); } void poke_blanked_console(void) { + WARN_CONSOLE_UNLOCKED(); + + /* This isn't perfectly race free, but a race here would be mostly harmless, + * at worse, we'll do a spurrious blank and it's unlikely + */ del_timer(&console_timer); + blank_timer_expired = 0; + if (ignore_poke || !vt_cons[fg_console] || vt_cons[fg_console]->vc_mode == KD_GRAPHICS) return; - if (console_blanked) { - console_timer.function = unblank_screen_t; - mod_timer(&console_timer, jiffies); /* Now */ - } else if (blankinterval) { + if (console_blanked) + unblank_screen(); + else if (blankinterval) { mod_timer(&console_timer, jiffies + blankinterval); + blank_state = blank_normal_wait; } } @@ -2821,6 +2862,8 @@ void poke_blanked_console(void) void set_palette(int currcons) { + WARN_CONSOLE_UNLOCKED(); + if (vcmode != KD_GRAPHICS) sw->con_set_palette(vc_cons[currcons].d, color_table); } @@ -2829,6 +2872,8 @@ static int set_get_cmap(unsigned char *a { int i, j, k; + WARN_CONSOLE_UNLOCKED(); + for (i = 0; i < 16; i++) if (set) { get_user(default_red[i], arg++); @@ -2860,12 +2905,24 @@ static int set_get_cmap(unsigned char *a int con_set_cmap(unsigned char *arg) { - return set_get_cmap (arg,1); + int rc; + + acquire_console_sem(); + rc = set_get_cmap (arg,1); + release_console_sem(); + + return rc; } int con_get_cmap(unsigned char *arg) { - return set_get_cmap (arg,0); + int rc; + + acquire_console_sem(); + rc = set_get_cmap (arg,0); + release_console_sem(); + + return rc; } void reset_palette(int currcons) @@ -2939,8 +2996,12 @@ int con_font_op(int currcons, struct con set = 1; } else if (op->op == KD_FONT_OP_GET) set = 0; - else - return sw->con_font_op(vc_cons[currcons].d, op); + else { + acquire_console_sem(); + rc = sw->con_font_op(vc_cons[currcons].d, op); + release_console_sem(); + return rc; + } if (op->data) { temp = kmalloc(size, GFP_KERNEL); if (!temp) @@ -3035,10 +3096,14 @@ static int pm_con_request(struct pm_dev switch (rqst) { case PM_RESUME: + acquire_console_sem(); unblank_screen(); + release_console_sem(); break; case PM_SUSPEND: + acquire_console_sem(); do_blank_screen(0); + release_console_sem(); break; } return 0; diff -puN drivers/char/vt_ioctl.c~vt-locking-fixes-2 drivers/char/vt_ioctl.c --- 25/drivers/char/vt_ioctl.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/drivers/char/vt_ioctl.c 2004-01-10 21:31:27.000000000 -0800 @@ -470,6 +470,9 @@ int vt_ioctl(struct tty_struct *tty, str * currently, setting the mode from KD_TEXT to KD_GRAPHICS * doesn't do a whole lot. i'm not sure if it should do any * restoration of modes or what... + * + * XXX It should at least call into the driver, fbdev's definitely + * need to restore their engine state. --BenH */ if (!perm) return -EPERM; @@ -492,10 +495,12 @@ int vt_ioctl(struct tty_struct *tty, str /* * explicitly blank/unblank the screen if switching modes */ + acquire_console_sem(); if (arg == KD_TEXT) unblank_screen(); else do_blank_screen(1); + release_console_sem(); return 0; case KDGETMODE: @@ -665,18 +670,29 @@ int vt_ioctl(struct tty_struct *tty, str return -EFAULT; if (tmp.mode != VT_AUTO && tmp.mode != VT_PROCESS) return -EINVAL; + acquire_console_sem(); vt_cons[console]->vt_mode = tmp; /* the frsig is ignored, so we set it to 0 */ vt_cons[console]->vt_mode.frsig = 0; vt_cons[console]->vt_pid = current->pid; /* no switch is required -- saw@shade.msu.ru */ vt_cons[console]->vt_newvt = -1; + release_console_sem(); return 0; } case VT_GETMODE: - return copy_to_user((void*)arg, &(vt_cons[console]->vt_mode), - sizeof(struct vt_mode)) ? -EFAULT : 0; + { + struct vt_mode tmp; + int rc; + + acquire_console_sem(); + memcpy(&tmp, &vt_cons[console]->vt_mode, sizeof(struct vt_mode)); + release_console_sem(); + + rc = copy_to_user((void*)arg, &tmp, sizeof(struct vt_mode)); + return rc ? -EFAULT : 0; + } /* * Returns global vt state. Note that VT 0 is always open, since @@ -718,7 +734,9 @@ int vt_ioctl(struct tty_struct *tty, str if (arg == 0 || arg > MAX_NR_CONSOLES) return -ENXIO; arg--; + acquire_console_sem(); i = vc_allocate(arg); + release_console_sem(); if (i) return i; set_console(arg); @@ -768,17 +786,20 @@ int vt_ioctl(struct tty_struct *tty, str * The current vt has been released, so * complete the switch. */ - int newvt = vt_cons[console]->vt_newvt; + int newvt; + acquire_console_sem(); + newvt = vt_cons[console]->vt_newvt; vt_cons[console]->vt_newvt = -1; i = vc_allocate(newvt); - if (i) + if (i) { + release_console_sem(); return i; + } /* * When we actually do the console switch, * make sure we are atomic with respect to * other console switches.. */ - acquire_console_sem(); complete_change_console(newvt); release_console_sem(); } @@ -806,16 +827,21 @@ int vt_ioctl(struct tty_struct *tty, str return -ENXIO; if (arg == 0) { /* disallocate all unused consoles, but leave 0 */ - for (i=1; iv_rows) || get_user(cc, &vtsizes->v_cols)) return -EFAULT; - for (i = 0; i < MAX_NR_CONSOLES; i++) + for (i = 0; i < MAX_NR_CONSOLES; i++) { + acquire_console_sem(); vc_resize(i, cc, ll); + release_console_sem(); + } return 0; } @@ -870,11 +899,13 @@ int vt_ioctl(struct tty_struct *tty, str for (i = 0; i < MAX_NR_CONSOLES; i++) { if (!vc_cons[i].d) continue; + acquire_console_sem(); if (vlin) vc_cons[i].d->vc_scan_lines = vlin; if (clin) vc_cons[i].d->vc_font.height = clin; vc_resize(i, cc, ll); + release_console_sem(); } return 0; } diff -puN include/linux/console.h~vt-locking-fixes-2 include/linux/console.h --- 25/include/linux/console.h~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/include/linux/console.h 2004-01-10 21:31:27.000000000 -0800 @@ -102,6 +102,14 @@ extern void acquire_console_sem(void); extern void release_console_sem(void); extern void console_conditional_schedule(void); extern void console_unblank(void); +extern int is_console_locked(void); + +/* Some debug stub to catch some of the obvious races in the VT code */ +#if 1 +#define WARN_CONSOLE_UNLOCKED() WARN_ON(!is_console_locked() && !oops_in_progress) +#else +#define WARN_CONSOLE_UNLOCKED() +#endif /* VESA Blanking Levels */ #define VESA_NO_BLANKING 0 diff -puN kernel/power/console.c~vt-locking-fixes-2 kernel/power/console.c --- 25/kernel/power/console.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/kernel/power/console.c 2004-01-10 21:31:27.000000000 -0800 @@ -6,6 +6,7 @@ #include #include +#include #include "power.h" static int new_loglevel = 10; @@ -18,14 +19,20 @@ int pm_prepare_console(void) console_loglevel = new_loglevel; #ifdef SUSPEND_CONSOLE + acquire_console_sem(); + orig_fgconsole = fg_console; - if (vc_allocate(SUSPEND_CONSOLE)) + if (vc_allocate(SUSPEND_CONSOLE)) { /* we can't have a free VC for now. Too bad, * we don't want to mess the screen for now. */ + release_console_sem(); return 1; + } set_console(SUSPEND_CONSOLE); + release_console_sem(); + if (vt_waitactive(SUSPEND_CONSOLE)) { pr_debug("Suspend: Can't switch VCs."); return 1; @@ -40,12 +47,9 @@ void pm_restore_console(void) { console_loglevel = orig_loglevel; #ifdef SUSPEND_CONSOLE + acquire_console_sem(); set_console(orig_fgconsole); - - /* FIXME: - * This following part is left over from swsusp. Is it really needed? - */ - update_screen(fg_console); + release_console_sem(); #endif return; } diff -puN kernel/printk.c~vt-locking-fixes-2 kernel/printk.c --- 25/kernel/printk.c~vt-locking-fixes-2 2004-01-10 21:31:27.000000000 -0800 +++ 25-akpm/kernel/printk.c 2004-01-10 21:31:27.000000000 -0800 @@ -62,6 +62,15 @@ int oops_in_progress; */ static DECLARE_MUTEX(console_sem); struct console *console_drivers; +/* + * This is used for debugging the mess that is the VT code by + * keeping track if we have the console semaphore held. It's + * definitely not the perfect debug tool (we don't know if _WE_ + * hold it are racing, but it helps tracking those weird code + * path in the console code where we end up in places I want + * locked without the console sempahore held + */ +static int console_locked; /* * logbuf_lock protects log_buf, log_start, log_end, con_start and logged_chars @@ -524,6 +533,7 @@ asmlinkage int printk(const char *fmt, . goto out; } if (!down_trylock(&console_sem)) { + console_locked = 1; /* * We own the drivers. We can drop the spinlock and let * release_console_sem() print the text @@ -557,10 +567,17 @@ void acquire_console_sem(void) if (in_interrupt()) BUG(); down(&console_sem); + console_locked = 1; console_may_schedule = 1; } EXPORT_SYMBOL(acquire_console_sem); +int is_console_locked(void) +{ + return console_locked; +} +EXPORT_SYMBOL(is_console_locked); + /** * release_console_sem - unlock the console system * @@ -592,12 +609,14 @@ void release_console_sem(void) spin_unlock_irqrestore(&logbuf_lock, flags); call_console_drivers(_con_start, _log_end); } + console_locked = 0; console_may_schedule = 0; up(&console_sem); spin_unlock_irqrestore(&logbuf_lock, flags); if (wake_klogd && !oops_in_progress && waitqueue_active(&log_wait)) wake_up_interruptible(&log_wait); } +EXPORT_SYMBOL(release_console_sem); /** console_conditional_schedule - yield the CPU if required * @@ -633,6 +652,7 @@ void console_unblank(void) */ if (down_trylock(&console_sem) != 0) return; + console_locked = 1; console_may_schedule = 0; for (c = console_drivers; c != NULL; c = c->next) if ((c->flags & CON_ENABLED) && c->unblank) _