Skip to contentSkip to conversation

xencons missing string allocation

PATCH In review Alex Williamson 4 participants 12 messages Dec 9, 2005 – Dec 13, 2005

lore
Share & tools
lore

Alex Williamson

author
#0 lore raw

PATCH 1/1 xencons missing string allocation

I was trying to boot dom0 w/ "xencons=ttyS1 console=ttyS1". It gives
some weird error messages:

Warning: dev (ttyS2) tty->count(2) != #fd's(1) in release_dev
Warning: dev (ttyS2) tty->count(3) != #fd's(1) in tty_open

And blows up with a page fault. The page fault is because we don't actually allocate a buffer for the tty driver name. The patch below fixes that problem. Using xencons=ttyS1 still doesn't quite work and prints lots of the above error messages, but at least it doesn't crash dom0 now. Patch vs xen-unstable.hg. Thanks,

Alex

Signed-off-by Alex Williamson <[email protected]>

---

linux-2.6-xen-sparse/drivers/xen/console/console.c +14 -2
diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
@@ -641,11 +641,23 @@
 
 	if (xc_mode == XC_SERIAL)
 	{
-		DRV(xencons_driver)->name        = "ttyS";
+		DRV(xencons_driver)->name = kmalloc(strlen("ttyS") + 1,
+		                                    GFP_KERNEL);
+		if (!DRV(xencons_driver)->name) {
+			kfree(xencons_driver);
+			return -ENOMEM;
+		}
+		strcpy(DRV(xencons_driver)->name, "ttyS");
 		DRV(xencons_driver)->minor_start = 64 + xc_num;
 		DRV(xencons_driver)->name_base   = 0 + xc_num;
 	} else {
-		DRV(xencons_driver)->name        = "tty";
+		DRV(xencons_driver)->name = kmalloc(strlen("tty") + 1,
+		                                    GFP_KERNEL);
+		if (!DRV(xencons_driver)->name) {
+			kfree(xencons_driver);
+			return -ENOMEM;
+		}
+		strcpy(DRV(xencons_driver)->name, "tty");
 		DRV(xencons_driver)->minor_start = xc_num;
 		DRV(xencons_driver)->name_base   = xc_num;
 	}
#1 lore raw
Quoted 9 lines · Alex Williamson

I was trying to boot dom0 w/ "xencons=ttyS1 console=ttyS1". It gives
some weird error messages:

Warning: dev (ttyS2) tty->count(2) != #fd's(1) in release_dev
Warning: dev (ttyS2) tty->count(3) != #fd's(1) in tty_open

And blows up with a page fault. The page fault is because we don't actually allocate a buffer for the tty driver name.

Errr... the patch looks curious. Why does it work when ->name points to the heap but not when it points to the data segment? they should be equivalent and many tty drivers appear to set ->name to the data segment. Is something trying to modify xencons_driver->name later?

(also, do you know why do we need the fugly DRV() macro in that code?)

Cheers,
Muli

Signature · 2 lines
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
author
#2 lore raw
Quoted 9 lines · Muli Ben-Yehuda
Quoted 7 lines · Alex Williamson

I was trying to boot dom0 w/ "xencons=ttyS1 console=ttyS1". It gives
some weird error messages:

Warning: dev (ttyS2) tty->count(2) != #fd's(1) in release_dev
Warning: dev (ttyS2) tty->count(3) != #fd's(1) in tty_open

Quoted 7 lines
Quoted 2 lines

And blows up with a page fault. The page fault is because we don't actually allocate a buffer for the tty driver name.

Errr... the patch looks curious. Why does it work when ->name points to the heap but not when it points to the data segment? they should be equivalent and many tty drivers appear to set ->name to the data segment. Is something trying to modify xencons_driver->name later?

You're right, I guess a lot of tty/char drivers seem to have the name
on the heap. However, it would suggest there's a path where the name is referenced outside of the context of that function since it prevents a page fault. I'll keep looking to make sure I'm not just getting lucky.

Quoted 1 line

(also, do you know why do we need the fugly DRV() macro in that code?)

No idea, I'm not a fan either. Thanks,

Alex

Signature · 1 line
Alex Williamson                             HP Linux & Open Source Lab
author
#3 lore raw
Quoted 18 lines · Alex Williamson
Quoted 9 lines · Muli Ben-Yehuda
Quoted 7 lines · Alex Williamson

I was trying to boot dom0 w/ "xencons=ttyS1 console=ttyS1". It gives
some weird error messages:

Warning: dev (ttyS2) tty->count(2) != #fd's(1) in release_dev
Warning: dev (ttyS2) tty->count(3) != #fd's(1) in tty_open

Quoted 7 lines
Quoted 2 lines

And blows up with a page fault. The page fault is because we don't actually allocate a buffer for the tty driver name.

Errr... the patch looks curious. Why does it work when ->name points to the heap but not when it points to the data segment? they should be equivalent and many tty drivers appear to set ->name to the data segment. Is something trying to modify xencons_driver->name later?

Quoted 4 lines

You're right, I guess a lot of tty/char drivers seem to have the name
on the heap. However, it would suggest there's a path where the name is referenced outside of the context of that function since it prevents a page fault. I'll keep looking to make sure I'm not just getting lucky.

Ok, disregard that previous attempt, it was definitely chasing a
false positive. I'm not sure this one is correct either, but I'll toss it out in case anyone else is interested in chasing this problem too. I believe the problem is that kcons_device() is incorrectly calculating the index when xc_num != 0 on serial devices. If I subtract xc_num from the console index, which should always give me 0, things work perfectly for all ttyS console values (that I've tested). I don't know if something similar needs to be done for tty devices. Patch attached, comments/suggestions welcome. Thanks,

Alex

Signed-off-by Alex Williamson <[email protected]>

---

linux-2.6-xen-sparse/drivers/xen/console/console.c +1 -1
diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
@@ -168,7 +168,7 @@
 
 static struct tty_driver *kcons_device(struct console *c, int *index)
 {
-	*index = c->index;
+	*index = c->index - (xc_mode == XC_SERIAL ? xc_num : 0);
 	return xencons_driver;
 }
#4 lore raw
Quoted 9 lines · Alex Williamson

Ok, disregard that previous attempt, it was definitely chasing a
false positive. I'm not sure this one is correct either, but I'll toss it out in case anyone else is interested in chasing this problem too. I believe the problem is that kcons_device() is incorrectly calculating the index when xc_num != 0 on serial devices. If I subtract xc_num from the console index, which should always give me 0, things work perfectly for all ttyS console values (that I've tested). I don't know if something similar needs to be done for tty devices. Patch attached, comments/suggestions welcome. Thanks,

This seems a very bizarre thing to have to do! What does this index value mean??

-- Keir

author
#5 lore raw
Quoted 14 lines · Keir Fraser
Quoted 11 lines · Alex Williamson

Ok, disregard that previous attempt, it was definitely chasing a
false positive. I'm not sure this one is correct either, but I'll toss it out in case anyone else is interested in chasing this problem too. I
believe the problem is that kcons_device() is incorrectly calculating the index when xc_num != 0 on serial devices. If I subtract xc_num from
the console index, which should always give me 0, things work perfectly for all ttyS console values (that I've tested). I don't know if something similar needs to be done for tty devices. Patch attached, comments/suggestions welcome. Thanks,

Quoted 2 lines

This seems a very bizarre thing to have to do! What does this index value mean??

The index is effectively the index into the array of ttyS devices.
For example, ttyS[1] == ttyS1. When I specify console=ttyS1, this value gets separated into driver "ttyS", index 1 in the console data structure. The xen console driver only knows how to deal with index 0. The patch I sent confines the namespace translation to one place, but I think a similar change could be done in places like the open function where it specifically checks for index == 0. Then again, it may even work as is if the user specifies "xencons=ttyS1 console=ttyS0" where ttyS0 would automatically become index 0 in the xen console driver. This of course seems far from intuitive and may break again if the kernel 8250 driver is loaded. My goal is to be able to include both the 8250 driver and the xen console driver and have them work together by specifying a xencons= value above the range of ports the 8250 driver claims. From Ian's commit log, I think this is supposed to work, but it currently doesn't. Thanks,

Alex

#6 lore raw
Quoted 8 lines

The index is effectively the index into the array of ttyS devices.
For example, ttyS[1] == ttyS1. When I specify console=ttyS1, this value gets separated into driver "ttyS", index 1 in the console data structure. The xen console driver only knows how to deal with index 0. The patch I sent confines the namespace translation to one place, but I think a similar change could be done in places like the open function where it specifically checks for index == 0.

Should the patch then not just set the index to zero, rather than conditionally subtracting xc_num?

-- Keir

author
#7 lore raw
Quoted 11 lines · Keir Fraser
Quoted 8 lines

The index is effectively the index into the array of ttyS devices.
For example, ttyS[1] == ttyS1. When I specify console=ttyS1, this value
gets separated into driver "ttyS", index 1 in the console data structure. The xen console driver only knows how to deal with index 0. The patch I sent confines the namespace translation to one place, but I think a similar change could be done in places like the open function where it specifically checks for index == 0.

Should the patch then not just set the index to zero, rather than conditionally subtracting xc_num?

You're right, but it's not quite that easy. I think we need some
consistency checking here. The point of this function seems to be determining if the driver owns the device. If so, set the index and return the driver, otherwise pass. I think the patch below does a better job of solving the problem. When xc_num == c->index, the device is the port xencons created, so the index is 0 and we claim it. The only slightly complicated one is when using the tty devices c->index is zero when xc_num is 1. I believe this does the right thing in all cases, let me know what you think. Thanks,

Alex

Signed-off-by Alex Williamson <[email protected]>

---

linux-2.6-xen-sparse/drivers/xen/console/console.c +6 -2
diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
@@ -168,8 +168,12 @@
 
 static struct tty_driver *kcons_device(struct console *c, int *index)
 {
-	*index = c->index;
-	return xencons_driver;
+	if (c->index == xc_num ||
+	    (xc_mode == XC_TTY && xc_num == 1 && !c->index)) {
+		*index = 0;
+		return xencons_driver;
+	}
+	return NULL;
 }
 
 static struct console kcons_info = {
#8 lore raw
Quoted 9 lines · Alex Williamson

You're right, but it's not quite that easy. I think we need some
consistency checking here. The point of this function seems to be determining if the driver owns the device. If so, set the index and return the driver, otherwise pass. I think the patch below does a better job of solving the problem. When xc_num == c->index, the device is the port xencons created, so the index is 0 and we claim it. The only slightly complicated one is when using the tty devices c->index is zero when xc_num is 1. I believe this does the right thing in all cases, let me know what you think. Thanks,

What I would like to know is what the two index values actually mean. :-) That is, what is c->index, and what is the index value that is returned? Without knowing this I have no idea whether your patch is correct or not. Is the expected behaviour of that console driver hook function understood and/or documented?

-- Keir

author
#9 lore raw
Quoted 12 lines · Keir Fraser
Quoted 9 lines · Alex Williamson

You're right, but it's not quite that easy. I think we need some
consistency checking here. The point of this function seems to be determining if the driver owns the device. If so, set the index and return the driver, otherwise pass. I think the patch below does a better job of solving the problem. When xc_num == c->index, the device is the port xencons created, so the index is 0 and we claim it. The only slightly complicated one is when using the tty devices c->index is zero when xc_num is 1. I believe this does the right thing in all cases, let me know what you think. Thanks,

Quoted 5 lines

What I would like to know is what the two index values actually mean. :-) That is, what is c->index, and what is the index value that is returned? Without knowing this I have no idea whether your patch is correct or not. Is the expected behaviour of that console driver hook function understood and/or documented?

I've had to do a bit more homework myself to rationalize these
changes, but I think I have a fuzzy idea how it all fits together. We start by creating a list of console_cmdlines via add_preferred_console(). This function is called for each console= boot option on the command line. The console_cmdline list stores the name, index and option for each console option, with the last entry being the preferred console. In this context, the index is the entry into the global address space for the name of the device. For instance, ttyS1 is name "ttyS", index 1.

Later on in boot, xen_console_init() calls register_console(). This
goes through the list of console_cmdlines looking for matches. The console structure that xen_console_init() passes in uses an index value of -1. The interpretation of values < 0 is that this driver claims all indexes of the given name. I suspect this is not what we want since the xen console driver only handles a single port. In this case, the name is matched and the index value specified on the command line is copied into the console structure. The console structures are added to the console_drivers list in LIFO order with the exception that the preferred console is always at the head of the list.

Even later in boot, the tty half of the xen console driver gets
started via xencons_init(). Here a tty driver and tty device are registered and where we get to the other kind of index. The device is registered as index zero into the array or devices this particular tty driver claims. This indicates the device lives at the major and minor_start values specified by the tty driver.

Finally, we get to the point where we want to open /dev/console.
tty_open() calls console_device() looking for a tty driver and tty index. console_device() starts at the head of the console_drivers list (the preferred console) and calls the console->device() function. The device function needs to return the tty driver associated with this device, and set the index to the offset of the device within the array of tty devices the driver claims.

As you can see, the xen console driver is a little bit unique here.
Most drivers have their own range of devices they claim and the tty index is identical to the device file index. For us, the tty index is always zero regardless of the device file index. Based on this understanding of how it all works, I think the below patch is sufficient to solve the problem and allow us to move xencons around to device numbers other than ttyS0 and tty1. Hopefully that clears up the index confusion. Thanks,

Alex

Signed-off-by Alex Williamson <[email protected]>

---

linux-2.6-xen-sparse/drivers/xen/console/console.c Tue Dec 13 13:18:00 2005 renamed from linux-2.6-xen-sparse/drivers/xen/console/console.c Fri Dec 9 11:05:06 2005 +3 -1
diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
@@ -168,7 +168,7 @@
 
 static struct tty_driver *kcons_device(struct console *c, int *index)
 {
-       *index = c->index;
+       *index = 0;
        return xencons_driver;
 }
 

@@ -212,6 +212,8 @@
        default:
                return __RETCODE;
        }
+
+       kcons_info.index = xc_num;
 
        wbuf = alloc_bootmem(wbuf_size);
#10 lore raw
Quoted 8 lines · Alex Williamson

As you can see, the xen console driver is a little bit unique here.
Most drivers have their own range of devices they claim and the tty index is identical to the device file index. For us, the tty index is always zero regardless of the device file index. Based on this understanding of how it all works, I think the below patch is sufficient to solve the problem and allow us to move xencons around to device numbers other than ttyS0 and tty1. Hopefully that clears up the index confusion. Thanks,

Thanks for investigating! I'll apply the patch.

-- Keir

#11 lore raw
In reply to Alex Williamson
Quoted 11 lines · Alex Williamson

I was trying to boot dom0 w/ "xencons=ttyS1 console=ttyS1". It gives
some weird error messages:

Warning: dev (ttyS2) tty->count(2) != #fd's(1) in release_dev
Warning: dev (ttyS2) tty->count(3) != #fd's(1) in tty_open

And blows up with a page fault. The page fault is because we don't actually allocate a buffer for the tty driver name. The patch below fixes that problem. Using xencons=ttyS1 still doesn't quite work and prints lots of the above error messages, but at least it doesn't crash dom0 now. Patch vs xen-unstable.hg. Thanks,

Why not using a static buffer ?

Tristan.