[Qemu-devel] [PATCH 0/2] A few race conditions in E1000 device fixed

[Qemu-devel] [PATCH 0/2] A few race conditions in E1000 device fixed

[Dmitry Fleytman]

Following patches fix a few race conditions in E1000 code:

  1st patch fixes race condition between driver shutdown and device shutdown (see patch comment)
  It also work-arounds race condition in e1000 Linux driver between RX enable and RX rings init
  (Separate patch for the second problem sent to e1000-devel/linux/kernel and accepted by maintainers, see
   http://sourceforge.net/mailarchive/forum.php?thread_name=1350280341.2152.12.camel%40jtkirshe-mobl&forum_name=e1000-devel)

  2nd patch is pretty trivial and adds forgotten field into live migration list thus fixing another race condition.

Dmitry Fleytman (2):
  Fix a race condition in E1000 device implementation:
  Fix a race condition in E1000 device live migration.     One of
    data-transfer related flags not in migrated fields list.

 hw/e1000.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 1/2] Fix a race condition in E1000 device implementation:

[Dmitry Fleytman]

Device driver enables RX on shutdown after HW reset
in case device is configured for wake on LAN.
Although RX is enabled corresponding rings are not initialized
and descrptor addresses of RX ring are invalid.
If packet arrives with proper timing QEMU crashes due to invalid
guest memory access attempt or guest memory gets corrupted.

Reported-by: Chris Webb <chris.webb@elastichosts.com>
Reported-by: Richard Davies <richard.davies@elastichosts.com>
Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/e1000.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 63fee10..1e66ecf 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -93,6 +93,7 @@ typedef struct E1000State_st {
     uint32_t rxbuf_size;
     uint32_t rxbuf_min_shift;
     int check_rxov;
+    uint32_t rx_init_done;
     struct e1000_tx {
         unsigned char header[256];
         unsigned char vlan_header[4];
@@ -267,6 +268,7 @@ static void e1000_reset(void *opaque)
 {
     E1000State *d = opaque;
 
+    d->check_rxov = 1;
     qemu_del_timer(d->autoneg_timer);
     memset(d->phy_reg, 0, sizeof d->phy_reg);
     memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
@@ -291,6 +293,12 @@ static void
 set_rx_control(E1000State *s, int index, uint32_t val)
 {
     s->mac_reg[RCTL] = val;
+
+    if (!(s->mac_reg[RCTL] & E1000_RCTL_EN)) {
+        s->rx_init_done = 0;
+        s->check_rxov = 1;
+    }
+
     s->rxbuf_size = rxbufsize(val);
     s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
     DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
@@ -925,8 +933,18 @@ mac_writereg(E1000State *s, int index, uint32_t val)
 static void
 set_rdt(E1000State *s, int index, uint32_t val)
 {
-    s->check_rxov = 0;
     s->mac_reg[index] = val & 0xffff;
+
+    if (s->mac_reg[index] || s->rx_init_done) {
+        s->check_rxov = 0;
+        /* This is a fix for RX initialization race
+        *  present in e1000 driver on some kernels.
+        *  We consider RX enabled only after we've seen
+        *  at least one RX descriptor filled by guest.
+        */
+        s->rx_init_done = 1;
+    }
+
     if (e1000_has_rxbufs(s, 1)) {
         qemu_flush_queued_packets(&s->nic->nc);
     }
@@ -1102,6 +1120,7 @@ static const VMStateDescription vmstate_e1000 = {
         VMSTATE_UNUSED(4), /* Was mmio_base.  */
         VMSTATE_UINT32(rxbuf_size, E1000State),
         VMSTATE_UINT32(rxbuf_min_shift, E1000State),
+        VMSTATE_UINT32(rx_init_done, E1000State),
         VMSTATE_UINT32(eecd_state.val_in, E1000State),
         VMSTATE_UINT16(eecd_state.bitnum_in, E1000State),
         VMSTATE_UINT16(eecd_state.bitnum_out, E1000State),
@@ -1269,6 +1288,7 @@ static int pci_e1000_init(PCIDevice *pci_dev)
     add_boot_device_path(d->conf.bootindex, &pci_dev->qdev, "/ethernet-phy@0");
 
     d->autoneg_timer = qemu_new_timer_ms(vm_clock, e1000_autoneg_timer, d);
+    d->check_rxov = 1;
 
     return 0;
 }
-- 
1.7.11.4

[Qemu-devel] [PATCH 2/2] Fix a race condition in E1000 device live migration. One of data-transfer related flags not in migrated fields list.

[Dmitry Fleytman]

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
---
 hw/e1000.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 1e66ecf..efbe0c9 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -92,7 +92,7 @@ typedef struct E1000State_st {
 
     uint32_t rxbuf_size;
     uint32_t rxbuf_min_shift;
-    int check_rxov;
+    uint32_t check_rxov;
     uint32_t rx_init_done;
     struct e1000_tx {
         unsigned char header[256];
@@ -1120,6 +1120,7 @@ static const VMStateDescription vmstate_e1000 = {
         VMSTATE_UNUSED(4), /* Was mmio_base.  */
         VMSTATE_UINT32(rxbuf_size, E1000State),
         VMSTATE_UINT32(rxbuf_min_shift, E1000State),
+        VMSTATE_UINT32(check_rxov, E1000State),
         VMSTATE_UINT32(rx_init_done, E1000State),
         VMSTATE_UINT32(eecd_state.val_in, E1000State),
         VMSTATE_UINT16(eecd_state.bitnum_in, E1000State),
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 0/2] A few race conditions in E1000 device fixed

[Dmitry Fleytman]

Hello,

Please, ignore 1st patch for now.

Although it fixes the problem observed it looks like there is a better
and easier solution (many thanks to Intel Guys that explained e1000
operation in details:
http://sourceforge.net/mailarchive/forum.php?thread_name=CAGHCxhcad%3Dzx7ihX5zoDB%3DZOLGGuZty%3DBck6zSoMQ-9S3ZJo7w%40mail.gmail.com&forum_name=e1000-devel).

We'll submit the final patch soon.

Dmitry.

On Mon, Oct 15, 2012 at 6:48 PM, Dmitry Fleytman <dmitry@daynix.com> wrote:
> Following patches fix a few race conditions in E1000 code:
>
>   1st patch fixes race condition between driver shutdown and device shutdown (see patch comment)
>   It also work-arounds race condition in e1000 Linux driver between RX enable and RX rings init
>   (Separate patch for the second problem sent to e1000-devel/linux/kernel and accepted by maintainers, see
>    http://sourceforge.net/mailarchive/forum.php?thread_name=1350280341.2152.12.camel%40jtkirshe-mobl&forum_name=e1000-devel)
>
>   2nd patch is pretty trivial and adds forgotten field into live migration list thus fixing another race condition.
>
> Dmitry Fleytman (2):
>   Fix a race condition in E1000 device implementation:
>   Fix a race condition in E1000 device live migration.     One of
>     data-transfer related flags not in migrated fields list.
>
>  hw/e1000.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> --
> 1.7.11.4
>



-- 
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman

Re: [Qemu-devel] [PATCH 2/2] Fix a race condition in E1000 device live migration. One of data-transfer related flags not in migrated fields list.

[Stefan Hajnoczi]

On Mon, Oct 15, 2012 at 06:48:53PM +0200, Dmitry Fleytman wrote:

The commit message is very long but the commit description is empty.
Please keep the message short and add the rest into the description.

> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> ---
>  hw/e1000.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 1e66ecf..efbe0c9 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -92,7 +92,7 @@ typedef struct E1000State_st {
>  
>      uint32_t rxbuf_size;
>      uint32_t rxbuf_min_shift;
> -    int check_rxov;
> +    uint32_t check_rxov;
>      uint32_t rx_init_done;
>      struct e1000_tx {
>          unsigned char header[256];
> @@ -1120,6 +1120,7 @@ static const VMStateDescription vmstate_e1000 = {
>          VMSTATE_UNUSED(4), /* Was mmio_base.  */
>          VMSTATE_UINT32(rxbuf_size, E1000State),
>          VMSTATE_UINT32(rxbuf_min_shift, E1000State),
> +        VMSTATE_UINT32(check_rxov, E1000State),

This breaks old -> new migration.  Please see docs/migration.txt on
VMSTATE and versions.  It might also be useful to use git-blame(1) on
some existing devices to see how people have modified the VMSTATE
without breaking migration (this is something I don't know much about).

Stefan

Re: [Qemu-devel] [PATCH 2/2] Fix a race condition in E1000 device live migration. One of data-transfer related flags not in migrated fields list.

[Kevin Wolf]

Am 16.10.2012 10:32, schrieb Stefan Hajnoczi:
> On Mon, Oct 15, 2012 at 06:48:53PM +0200, Dmitry Fleytman wrote:
> 
> The commit message is very long but the commit description is empty.
> Please keep the message short and add the rest into the description.
> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> ---
>>  hw/e1000.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/e1000.c b/hw/e1000.c
>> index 1e66ecf..efbe0c9 100644
>> --- a/hw/e1000.c
>> +++ b/hw/e1000.c
>> @@ -92,7 +92,7 @@ typedef struct E1000State_st {
>>  
>>      uint32_t rxbuf_size;
>>      uint32_t rxbuf_min_shift;
>> -    int check_rxov;
>> +    uint32_t check_rxov;
>>      uint32_t rx_init_done;
>>      struct e1000_tx {
>>          unsigned char header[256];
>> @@ -1120,6 +1120,7 @@ static const VMStateDescription vmstate_e1000 = {
>>          VMSTATE_UNUSED(4), /* Was mmio_base.  */
>>          VMSTATE_UINT32(rxbuf_size, E1000State),
>>          VMSTATE_UINT32(rxbuf_min_shift, E1000State),
>> +        VMSTATE_UINT32(check_rxov, E1000State),
> 
> This breaks old -> new migration.  Please see docs/migration.txt on
> VMSTATE and versions.  It might also be useful to use git-blame(1) on
> some existing devices to see how people have modified the VMSTATE
> without breaking migration (this is something I don't know much about).

I think you'll want to use subsections anyway because they also fix the
new -> old case as good as they can: If the common case works correctly,
you can use a subsection to send the new state only in the special case.

Kevin