[Qemu-devel] virtio-serial NULL deference

[Qemu-devel] virtio-serial NULL deference

[Juan Quintela]

Hi Amit

Checking migration, I just found this problem:

I don't know what to put there.  a return -EINVAL or continue?
Looking more at the code, I am not sure what checks:

a- that bus->max_nr_ports is the same in both sides (or at least bigger
   on migration destination)
b- We sent the value of config.nr_ports, but ... we assign it back on
   destination, instead of checking that they are the same.
c- port->id is taken from nr_ports again, and nothing checks that ports
   appear in the same order in source and destination.

Throughts?

Later, Juan.

diff --cc configure
index cab1941,83f8b26..0000000
--- a/configure
+++ b/configure
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index d0e0219..7d2f0b9 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -429,6 +429,10 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
         id = qemu_get_be32(f);
         port = find_port_by_id(s, id);

+        if (port == NULL) {
+            return -EINVAL;
+        }
+
         port->guest_connected = qemu_get_byte(f);
     }

[Qemu-devel] Re: virtio-serial NULL deference

[Amit Shah]

On (Tue) Mar 09 2010 [14:15:45], Juan Quintela wrote:
> 
> Hi Amit

Hey Juan,

> Checking migration, I just found this problem:
> 
> I don't know what to put there.  a return -EINVAL or continue?
> Looking more at the code, I am not sure what checks:
> 
> a- that bus->max_nr_ports is the same in both sides (or at least bigger
>    on migration destination)

Yes, we should check for this.

> b- We sent the value of config.nr_ports, but ... we assign it back on
>    destination, instead of checking that they are the same.

This is done to accomodate for hot-plug/unplug. nr_ports will go up /
down after those operations. (Current implementation only increases this
value, on hotplug operations. On hot-unplug, this value is not
decremented.)

> c- port->id is taken from nr_ports again, and nothing checks that ports
>    appear in the same order in source and destination.

Actually, this has me thinking about how would this work:
- start vm with 3 ports
- hotplug 2 more ports
- migrate

Would the destination have 5 ports, or would it have 3? I thought qdev
would take care of this scenario (hotplug). I don't think I've tested
this, so I'll do this soon, but in case anyone knows the answer, please
let me know.

[snipped patch that's necessary in case qdev doesn't handle this kind of
hotplug]

		Amit
-- 
http://log.amitshah.net/

[Qemu-devel] Re: virtio-serial NULL deference

[Amit Shah]

On (Tue) Mar 09 2010 [20:59:58], Amit Shah wrote:
> On (Tue) Mar 09 2010 [14:15:45], Juan Quintela wrote:
> > 
> > Hi Amit
> 
> Hey Juan,
> 
> > Checking migration, I just found this problem:
> > 
> > I don't know what to put there.  a return -EINVAL or continue?
> > Looking more at the code, I am not sure what checks:
> > 
> > a- that bus->max_nr_ports is the same in both sides (or at least bigger
> >    on migration destination)
> 
> Yes, we should check for this.

I've added this check in my local tree.

> > b- We sent the value of config.nr_ports, but ... we assign it back on
> >    destination, instead of checking that they are the same.
> 
> This is done to accomodate for hot-plug/unplug. nr_ports will go up /
> down after those operations. (Current implementation only increases this
> value, on hotplug operations. On hot-unplug, this value is not
> decremented.)

For now I've added a check that tests nr_ports == s->config.nr_ports. If
that's not true, then return with -EINVAL.

These two were small changes, no problem.

> > c- port->id is taken from nr_ports again, and nothing checks that ports
> >    appear in the same order in source and destination.
> 
> Actually, this has me thinking about how would this work:
> - start vm with 3 ports
> - hotplug 2 more ports
> - migrate
> 
> Would the destination have 5 ports, or would it have 3? I thought qdev
> would take care of this scenario (hotplug). I don't think I've tested
> this, so I'll do this soon, but in case anyone knows the answer, please
> let me know.

I spoke with Dan and he confirmed libvirt starts qemu instances on the
destination computer with the appropriate devices, taking into
consideration the hotplug/unplug status.

However, the only problem here is virtio-serial ports are allocated an
'id', ie., a port number, and this is auto-assigned when a new port is
found by adding 1 to the previous id.

To make this whole thing independent of the order in which ports are
added / removed, we'll have to accept the port id on the command line.

This also means that we'll have to let the kernel know of the id because
the control messages that the kernel and qemu exchange contain the port
id.

So basically some design changes will have to be incorporated both, in
the kernel as well as in qemu to accomodate this. If this sounds right,
I'll get to this right away. If there's an easier solution, please let
me know.

		Amit