Skip to contentSkip to conversation

drivers/char/vt possible race

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages Feb 10, 2004 Char and misc drivers

lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 drivers/char/vt possible race

Hi !

I falled again on the crash in con_do_write() with driver->data beeing NULL. It happens during boot, when userland is playing open/close games with tty's, I was intentionally typing keys like mad during boot trying to trigger another problem when this one poped up.

Looking at the code, I'm not sure how protected we are by the above (tty) layer, paulus told me to not rely on anything like locking coming from there, so I decided to extend the scope of the console semaphore one more bit to cover races between calls to con_open, con_close and con_write. Note that in con_do_write, I intentionally drop the semaphore to avoid keeping it held when waiting on the local buffer, and I added some sanity checks on tty->driver_data with some printk's in case we still have an open race by the tty layer. At least, now, the couple vc_allocated & tty->driver_data should be protected though.

Andrew: I suggest putting that in -mm for a while, and if it doesn't trigger any new problem, upstream, maybe without my 2 printk's "argh" :)

Ben.

drivers/char/vt.c +29 -5
diff -urN linux-2.5/drivers/char/vt.c linuxppc-2.5-benh/drivers/char/vt.c
@@ -1883,14 +1883,24 @@
 	int c, tc, ok, n = 0, draw_x = -1;
 	unsigned int currcons;
 	unsigned long draw_from = 0, draw_to = 0;
-	struct vt_struct *vt = (struct vt_struct *)tty->driver_data;
+	struct vt_struct *vt;
 	u16 himask, charmask;
 	const unsigned char *orig_buf = NULL;
 	int orig_count;
 
 	if (in_interrupt())
 		return count;
-		
+
+	might_sleep();
+
+	acquire_console_sem();
+	vt = (struct vt_struct *)tty->driver_data;
+	if (vt == NULL) {
+		printk(KERN_ERR "vt: argh, driver_data is NULL !\n");
+		release_console_sem();
+		return 0;
+	}
+
 	currcons = vt->vc_num;
 	if (!vc_cons_allocated(currcons)) {
 	    /* could this happen? */

@@ -1899,13 +1909,16 @@
 		error = 1;
 		printk("con_write: tty %d not allocated\n", currcons+1);
 	    }
+	    release_console_sem();
 	    return 0;
 	}
+	release_console_sem();
 
 	orig_buf = buf;
 	orig_count = count;
 
 	if (from_user) {
+
 		down(&con_buf_sem);
 
 again:

@@ -1929,6 +1942,13 @@
 
 	acquire_console_sem();
 
+	vt = (struct vt_struct *)tty->driver_data;
+	if (vt == NULL) {
+		printk(KERN_ERR "vt: argh, driver_data _became_ NULL !\n");
+		release_console_sem();
+		goto out;
+	}
+
 	himask = hi_font_mask;
 	charmask = himask ? 0x1ff : 0xff;
 

@@ -2442,14 +2462,16 @@
 
 	acquire_console_sem();
 	i = vc_allocate(currcons);
-	release_console_sem();
-	if (i)
+	if (i) {
+		release_console_sem();
 		return i;
-
+	}
 	vt_cons[currcons]->vc_num = currcons;
 	tty->driver_data = vt_cons[currcons];
 	vc_cons[currcons].d->vc_tty = tty;
 
+	release_console_sem();
+
 	if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 		tty->winsize.ws_row = video_num_lines;
 		tty->winsize.ws_col = video_num_columns;

@@ -2467,10 +2489,12 @@
 		return;
 
 	vcs_remove_devfs(tty);
+	acquire_console_sem();
 	vt = (struct vt_struct*)tty->driver_data;
 	if (vt)
 		vc_cons[vt->vc_num].d->vc_tty = NULL;
 	tty->driver_data = 0;
+	release_console_sem();
 }
 
 static void vc_init(unsigned int currcons, unsigned int rows, unsigned int cols, int do_clear)
#1 lore raw
Quoted 8 lines · Benjamin Herrenschmidt

Hi !

I falled again on the crash in con_do_write() with driver->data beeing NULL. It happens during boot, when userland is playing open/close games with tty's, I was intentionally typing keys like mad during boot trying to trigger another problem when this one poped up.

OK. Was this patch confirmed to prevent any reoccurrences?

Quoted 3 lines

Andrew: I suggest putting that in -mm for a while, and if it doesn't trigger any new problem, upstream, maybe without my 2 printk's "argh" :)

Yup. I'll also bring back the sysfs patch which somehow triggers this race.

author
#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 8 lines · Benjamin Herrenschmidt

Hi !

I falled again on the crash in con_do_write() with driver->data beeing NULL. It happens during boot, when userland is playing open/close games with tty's, I was intentionally typing keys like mad during boot trying to trigger another problem when this one poped up.

OK. Was this patch confirmed to prevent any reoccurrences?

Well, I didn't see it again, and if for some reason, we still enter the function with tty->driver_data == NULL (which may still happen if the tty layer itself isn't serializing, which I suspect), we will print a warning and bail out.

In the end, I suppose the warning can be removed, but I want to make sure that if the race still happens, we behave properly now.

The patch makes sure the vt internal state stays consistent.

Quoted 6 lines
Quoted 3 lines

Andrew: I suggest putting that in -mm for a while, and if it doesn't trigger any new problem, upstream, maybe without my 2 printk's "argh" :)

Yup. I'll also bring back the sysfs patch which somehow triggers this race.

Yup, let me know.

Ben.