[PATCH] xencons missing string allocation

[PATCH] xencons missing string allocation

[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,

	Alex

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
--- a/linux-2.6-xen-sparse/drivers/xen/console/console.c	Fri Dec  9 11:05:06 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/console/console.c	Fri Dec  9 11:12:04 2005
@@ -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;
 	}

Re: [PATCH] xencons missing string allocation

[Muli Ben-Yehuda]

On Fri, Dec 09, 2005 at 11:37:31AM -0700, Alex Williamson wrote:

> 
>    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
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH] xencons missing string allocation

[Alex Williamson]

On Fri, 2005-12-09 at 20:54 +0200, Muli Ben-Yehuda wrote:
> On Fri, Dec 09, 2005 at 11:37:31AM -0700, Alex Williamson wrote:
> 
> > 
> >    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?

   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.

> (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

-- 
Alex Williamson                             HP Linux & Open Source Lab

Re: [PATCH] xencons missing string allocation

[Alex Williamson]

On Fri, 2005-12-09 at 13:37 -0700, Alex Williamson wrote:
> On Fri, 2005-12-09 at 20:54 +0200, Muli Ben-Yehuda wrote:
> > On Fri, Dec 09, 2005 at 11:37:31AM -0700, Alex Williamson wrote:
> > 
> > > 
> > >    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?
> 
>    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 <alex.williamson@hp.com>
---

diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
--- a/linux-2.6-xen-sparse/drivers/xen/console/console.c	Fri Dec  9 11:05:06 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/console/console.c	Fri Dec  9 16:34:33 2005
@@ -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;
 }

Re: [PATCH] xencons missing string allocation

[Keir Fraser]

On 10 Dec 2005, at 00:00, Alex Williamson wrote:

>    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

Re: [PATCH] xencons missing string allocation

[Alex Williamson]

On Sat, 2005-12-10 at 15:06 +0000, Keir Fraser wrote:
> On 10 Dec 2005, at 00:00, Alex Williamson wrote:
> 
> >    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??

   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

Re: [PATCH] xencons missing string allocation

[Keir Fraser]

>    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

Re: [PATCH] xencons missing string allocation

[Tristan Gingold]

Le Vendredi 09 Décembre 2005 19:37, Alex Williamson a écrit :
>    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.

Re: [PATCH] xencons missing string allocation

[Alex Williamson]

On Sat, 2005-12-10 at 23:12 +0000, Keir Fraser wrote:
> >    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 <alex.williamson@hp.com>
---

diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
--- a/linux-2.6-xen-sparse/drivers/xen/console/console.c	Fri Dec  9 11:05:06 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/console/console.c	Mon Dec 12 14:30:53 2005
@@ -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 = {

Re: [PATCH] xencons missing string allocation

[Keir Fraser]

On 12 Dec 2005, at 22:00, Alex Williamson wrote:

>    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

Re: [PATCH] xencons missing string allocation

[Alex Williamson]

On Tue, 2005-12-13 at 01:57 +0000, Keir Fraser wrote:
> On 12 Dec 2005, at 22:00, Alex Williamson wrote:
> 
> >    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?

  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 <alex.williamson@hp.com>
---

diff -r 53cff3f88e45 linux-2.6-xen-sparse/drivers/xen/console/console.c
--- a/linux-2.6-xen-sparse/drivers/xen/console/console.c        Fri Dec  9 11:05:06 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/console/console.c        Tue Dec 13 13:18:00 2005
@@ -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);

Re: [PATCH] xencons missing string allocation

[Keir Fraser]

On 13 Dec 2005, at 20:44, Alex Williamson wrote:

>    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