[Qemu-devel] [PATCH 0/3] slirp: Fix for requeuing crash, cleanups

[Qemu-devel] [PATCH 0/3] slirp: Fix for requeuing crash, cleanups

[Jan Kiszka]

This is an alternative, more complete approach to fix the requeuing-
related crashes reported recently. See patch 2 for details. The rest are
simple cleanups.

Please check carefully if I messed something up.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Jan Kiszka (3):
  slirp: Clean up ifs_init
  slirp: Fix requeuing of batchq packets in if_start
  slirp: Refactor if_start

 slirp/if.c   |  107 +++++++++++++++++++++++++++------------------------------
 slirp/if.h   |    2 -
 slirp/mbuf.h |    5 +++
 3 files changed, 56 insertions(+), 58 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/3] slirp: Clean up ifs_init

[Jan Kiszka]

Remove duplicate ifs_init macros, reimplement the logic as static inline
in mbuf.h.

CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c   |    2 --
 slirp/if.h   |    2 --
 slirp/mbuf.h |    5 +++++
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 2852396..8e0cac2 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -8,8 +8,6 @@
 #include <slirp.h>
 #include "qemu-timer.h"
 
-#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
-
 static void
 ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
 {
diff --git a/slirp/if.h b/slirp/if.h
index 2dac1c7..3327023 100644
--- a/slirp/if.h
+++ b/slirp/if.h
@@ -20,6 +20,4 @@
 /* 2 for alignment, 14 for ethernet, 40 for TCP/IP */
 #define IF_MAXLINKHDR (2 + 14 + 40)
 
-#define ifs_init(ifm) ((ifm)->ifs_next = (ifm)->ifs_prev = (ifm))
-
 #endif
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 0708840..8d7951f 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -124,4 +124,9 @@ void m_adj(struct mbuf *, int);
 int m_copy(struct mbuf *, struct mbuf *, int, int);
 struct mbuf * dtom(Slirp *, void *);
 
+static inline void ifs_init(struct mbuf *ifm)
+{
+    ifm->ifs_next = ifm->ifs_prev = ifm;
+}
+
 #endif
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Jan Kiszka]

In case we requeued a packet that was the head of a longer session
queue, we failed to restore this ordering. Also, we did not properly
deal with changes to Slirp::next_m.

Instead of a cumbersome roll back, this fix simply avoids any changes
until we know if the packet was actually sent. Both fixes crashes due
to inconsistent queues and simplifies the logic.

Thanks to Zhi Yong Wu who found the reason for these crashes.

CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   35 +++++++++++++++++++----------------
 1 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 8e0cac2..710ec23 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -154,6 +154,7 @@ if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
     int requeued = 0;
+    bool from_batchq = false;
 	struct mbuf *ifm, *ifqt;
 
 	DEBUG_CALL("if_start");
@@ -179,13 +180,26 @@ if_start(Slirp *slirp)
 		else
 		   ifm = slirp->if_batchq.ifq_next;
 
-		/* Set which packet to send on next iteration */
-		slirp->next_m = ifm->ifq_next;
+                from_batchq = true;
 	}
+
+        slirp->if_queued--;
+
+        /* Try to send packet unless it already expired */
+        if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
+            /* Packet is delayed due to pending ARP resolution */
+            requeued++;
+            goto out;
+        }
+
+        if (from_batchq) {
+            /* Set which packet to send on next iteration */
+            slirp->next_m = ifm->ifq_next;
+        }
+
 	/* Remove it from the queue */
 	ifqt = ifm->ifq_prev;
 	remque(ifm);
-	slirp->if_queued--;
 
 	/* If there are more packets for this session, re-queue them */
 	if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) {
@@ -200,20 +214,9 @@ if_start(Slirp *slirp)
 		   ifm->ifq_so->so_nqueued = 0;
 	}
 
-        if (ifm->expiration_date < now) {
-            /* Expired */
-            m_free(ifm);
-        } else {
-            /* Encapsulate the packet for sending */
-            if (if_encap(slirp, ifm)) {
-                m_free(ifm);
-            } else {
-                /* re-queue */
-                insque(ifm, ifqt);
-                requeued++;
-            }
-        }
+        m_free(ifm);
 
+ out:
 	if (slirp->if_queued)
 	   goto again;
 
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Stefan Weil]

Am 17.02.2012 16:45, schrieb Jan Kiszka:
> In case we requeued a packet that was the head of a longer session
> queue, we failed to restore this ordering. Also, we did not properly
> deal with changes to Slirp::next_m.
>
> Instead of a cumbersome roll back, this fix simply avoids any changes
> until we know if the packet was actually sent. Both fixes crashes due
> to inconsistent queues and simplifies the logic.
>
> Thanks to Zhi Yong Wu who found the reason for these crashes.
>
> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> CC: Fabien Chouteau <chouteau@adacore.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> slirp/if.c | 35 +++++++++++++++++++----------------
> 1 files changed, 19 insertions(+), 16 deletions(-)

Latest QEMU crashed here 4 times with MIPS Malta
when I tried 'apt-get update' in the guest. See gdb output
below for details.

I only got the crash with big endian MIPS, not with little
endian which is strange.

After I reverted the above patch, MIPS Malta worked
again as before.

So maybe we changed one crash against a new one.

Regards,

Stefan Weil


Program received signal SIGSEGV, Segmentation fault.
0x000055555577b2a4 in ifs_insque (ifm=0x555556eb0e10, ifmhead=0x0) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/if.c:14
14              ifm->ifs_next = ifmhead->ifs_next;
(gdb) p ifmhead
$1 = (struct mbuf *) 0x0
(gdb) l
9       #include "qemu-timer.h"
10
11      static void
12      ifs_insque(struct mbuf *ifm, struct mbuf *ifmhead)
13      {
14              ifm->ifs_next = ifmhead->ifs_next;
15              ifmhead->ifs_next = ifm;
16              ifm->ifs_prev = ifmhead;
17              ifm->ifs_next->ifs_prev = ifm;
18      }
(gdb) i s
#0  0x000055555577b2a4 in ifs_insque (ifm=0x555556eb0e10, ifmhead=0x0) 
at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/if.c:14
#1  0x000055555577b487 in if_output (so=0x555556ea0bd0, 
ifm=0x555556eb0e10) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/if.c:81
#2  0x000055555577cf2c in ip_output (so=0x555556ea0bd0, 
m0=0x555556eb0e10) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/ip_output.c:84
#3  0x00005555557863f2 in tcp_output (tp=0x555556ea0c80) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/tcp_output.c:456
#4  0x000055555577fd66 in slirp_select_poll (readfds=0x7fffffffda10, 
writefds=0x7fffffffda90, xfds=0x7fffffffdb10, select_error=0)
     at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/slirp.c:477
#5  0x000055555572d8c0 in main_loop_wait (nonblocking=1) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/main-loop.c:469
#6  0x0000555555721a61 in main_loop () at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/vl.c:1558
#7  0x00005555557284a2 in main (argc=25, argv=0x7fffffffdfe8, 
envp=0x7fffffffe0b8) at /home/stefan/src/qemu/repo.or.cz/qemu/ar7/vl.c:3667
(gdb) up
#1  0x000055555577b487 in if_output (so=0x555556ea0bd0, 
ifm=0x555556eb0e10) at 
/home/stefan/src/qemu/repo.or.cz/qemu/ar7/slirp/if.c:81
81                              ifs_insque(ifm, ifq->ifs_prev);
(gdb) p *ifq
$2 = {m_hdr = {mh_next = 0x5555564bfc48, mh_prev = 0x0, mh_nextpkt = 
0x0, mh_prevpkt = 0x0, mh_flags = 0, mh_size = 1562, mh_so = 
0x555556ea0bd0, mh_data = 0x555556eb0ea0 "E",
     mh_len = 1500}, slirp = 0x5555564bfb80, arp_requested = false, 
expiration_date = 18446744073709551615, M_dat = {m_dat_ = "", m_ext_ = 0x0}}

Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]

On 2012-02-28 23:18, Stefan Weil wrote:
> Am 17.02.2012 16:45, schrieb Jan Kiszka:
>> In case we requeued a packet that was the head of a longer session
>> queue, we failed to restore this ordering. Also, we did not properly
>> deal with changes to Slirp::next_m.
>>
>> Instead of a cumbersome roll back, this fix simply avoids any changes
>> until we know if the packet was actually sent. Both fixes crashes due
>> to inconsistent queues and simplifies the logic.
>>
>> Thanks to Zhi Yong Wu who found the reason for these crashes.
>>
>> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> CC: Fabien Chouteau <chouteau@adacore.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> slirp/if.c | 35 +++++++++++++++++++----------------
>> 1 files changed, 19 insertions(+), 16 deletions(-)
> 
> Latest QEMU crashed here 4 times with MIPS Malta
> when I tried 'apt-get update' in the guest. See gdb output
> below for details.
> 
> I only got the crash with big endian MIPS, not with little
> endian which is strange.
> 
> After I reverted the above patch, MIPS Malta worked
> again as before.
> 
> So maybe we changed one crash against a new one.

Embarrassing.

Does this help? Specifically expired packet handling is broken.

diff --git a/slirp/if.c b/slirp/if.c
index 33f08e1..954ef1e 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -153,7 +153,7 @@ void if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
     int requeued = 0;
-    bool from_batchq = false;
+    bool from_batchq;
     struct mbuf *ifm, *ifqt;
 
     DEBUG_CALL("if_start");
@@ -169,6 +169,7 @@ void if_start(Slirp *slirp)
          */
         if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
             ifm = slirp->if_fastq.ifq_next;
+            from_batchq = false;
         } else {
             /* Nothing on fastq, see if next_m is valid */
             if (slirp->next_m != &slirp->if_batchq) {
@@ -176,14 +177,17 @@ void if_start(Slirp *slirp)
             } else {
                 ifm = slirp->if_batchq.ifq_next;
             }
-
             from_batchq = true;
         }
 
         slirp->if_queued--;
 
         /* Try to send packet unless it already expired */
-        if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
+        if (ifm->expiration_date < now) {
+            m_free(ifm);
+            continue;
+        }
+        if (!if_encap(slirp, ifm)) {
             /* Packet is delayed due to pending ARP resolution */
             requeued++;
             continue;

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On 2012-02-28 23:52, Jan Kiszka wrote:
> On 2012-02-28 23:18, Stefan Weil wrote:
>> Am 17.02.2012 16:45, schrieb Jan Kiszka:
>>> In case we requeued a packet that was the head of a longer session
>>> queue, we failed to restore this ordering. Also, we did not properly
>>> deal with changes to Slirp::next_m.
>>>
>>> Instead of a cumbersome roll back, this fix simply avoids any changes
>>> until we know if the packet was actually sent. Both fixes crashes due
>>> to inconsistent queues and simplifies the logic.
>>>
>>> Thanks to Zhi Yong Wu who found the reason for these crashes.
>>>
>>> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>> CC: Fabien Chouteau <chouteau@adacore.com>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>> slirp/if.c | 35 +++++++++++++++++++----------------
>>> 1 files changed, 19 insertions(+), 16 deletions(-)
>>
>> Latest QEMU crashed here 4 times with MIPS Malta
>> when I tried 'apt-get update' in the guest. See gdb output
>> below for details.
>>
>> I only got the crash with big endian MIPS, not with little
>> endian which is strange.
>>
>> After I reverted the above patch, MIPS Malta worked
>> again as before.
>>
>> So maybe we changed one crash against a new one.
> 
> Embarrassing.
> 
> Does this help? Specifically expired packet handling is broken.
> 
> diff --git a/slirp/if.c b/slirp/if.c
> index 33f08e1..954ef1e 100644
> --- a/slirp/if.c
> +++ b/slirp/if.c
> @@ -153,7 +153,7 @@ void if_start(Slirp *slirp)
>  {
>      uint64_t now = qemu_get_clock_ns(rt_clock);
>      int requeued = 0;
> -    bool from_batchq = false;
> +    bool from_batchq;
>      struct mbuf *ifm, *ifqt;
>  
>      DEBUG_CALL("if_start");
> @@ -169,6 +169,7 @@ void if_start(Slirp *slirp)
>           */
>          if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
>              ifm = slirp->if_fastq.ifq_next;
> +            from_batchq = false;
>          } else {
>              /* Nothing on fastq, see if next_m is valid */
>              if (slirp->next_m != &slirp->if_batchq) {
> @@ -176,14 +177,17 @@ void if_start(Slirp *slirp)
>              } else {
>                  ifm = slirp->if_batchq.ifq_next;
>              }
> -
>              from_batchq = true;
>          }
>  
>          slirp->if_queued--;
>  
>          /* Try to send packet unless it already expired */
> -        if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
> +        if (ifm->expiration_date < now) {
> +            m_free(ifm);
> +            continue;
> +        }
> +        if (!if_encap(slirp, ifm)) {
>              /* Packet is delayed due to pending ARP resolution */
>              requeued++;
>              continue;
> 
> Jan

Nonsense. We need to walk the queues properly and carefully. Will come
up with a better version.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH 2/3] slirp: Fix requeuing of batchq packets in if_start

[Jan Kiszka]

On 2012-02-28 23:18, Stefan Weil wrote:
> Am 17.02.2012 16:45, schrieb Jan Kiszka:
>> In case we requeued a packet that was the head of a longer session
>> queue, we failed to restore this ordering. Also, we did not properly
>> deal with changes to Slirp::next_m.
>>
>> Instead of a cumbersome roll back, this fix simply avoids any changes
>> until we know if the packet was actually sent. Both fixes crashes due
>> to inconsistent queues and simplifies the logic.
>>
>> Thanks to Zhi Yong Wu who found the reason for these crashes.
>>
>> CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> CC: Fabien Chouteau <chouteau@adacore.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> slirp/if.c | 35 +++++++++++++++++++----------------
>> 1 files changed, 19 insertions(+), 16 deletions(-)
> 
> Latest QEMU crashed here 4 times with MIPS Malta
> when I tried 'apt-get update' in the guest. See gdb output
> below for details.
> 
> I only got the crash with big endian MIPS, not with little
> endian which is strange.
> 
> After I reverted the above patch, MIPS Malta worked
> again as before.
> 
> So maybe we changed one crash against a new one.

Could you retry with

git://git.kiszka.org/qemu.git queues/slirp

?

TIA,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 3/3] slirp: Refactor if_start

[Jan Kiszka]

Replace gotos with a while loop, fix coding style.

CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   78 +++++++++++++++++++++++++++--------------------------------
 1 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 710ec23..33f08e1 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -149,39 +149,36 @@ diddit:
  * from the second session, then one packet from the third, then back
  * to the first, etc. etc.
  */
-void
-if_start(Slirp *slirp)
+void if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
     int requeued = 0;
     bool from_batchq = false;
-	struct mbuf *ifm, *ifqt;
-
-	DEBUG_CALL("if_start");
+    struct mbuf *ifm, *ifqt;
 
-	if (slirp->if_queued == 0)
-	   return; /* Nothing to do */
+    DEBUG_CALL("if_start");
 
- again:
+    while (slirp->if_queued) {
         /* check if we can really output */
         if (!slirp_can_output(slirp->opaque))
             return;
 
-	/*
-	 * See which queue to get next packet from
-	 * If there's something in the fastq, select it immediately
-	 */
-	if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
-		ifm = slirp->if_fastq.ifq_next;
-	} else {
-		/* Nothing on fastq, see if next_m is valid */
-		if (slirp->next_m != &slirp->if_batchq)
-		   ifm = slirp->next_m;
-		else
-		   ifm = slirp->if_batchq.ifq_next;
-
-                from_batchq = true;
-	}
+        /*
+         * See which queue to get next packet from
+         * If there's something in the fastq, select it immediately
+         */
+        if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
+            ifm = slirp->if_fastq.ifq_next;
+        } else {
+            /* Nothing on fastq, see if next_m is valid */
+            if (slirp->next_m != &slirp->if_batchq) {
+                ifm = slirp->next_m;
+            } else {
+                ifm = slirp->if_batchq.ifq_next;
+            }
+
+            from_batchq = true;
+        }
 
         slirp->if_queued--;
 
@@ -189,7 +186,7 @@ if_start(Slirp *slirp)
         if (ifm->expiration_date >= now && !if_encap(slirp, ifm)) {
             /* Packet is delayed due to pending ARP resolution */
             requeued++;
-            goto out;
+            continue;
         }
 
         if (from_batchq) {
@@ -197,28 +194,25 @@ if_start(Slirp *slirp)
             slirp->next_m = ifm->ifq_next;
         }
 
-	/* Remove it from the queue */
-	ifqt = ifm->ifq_prev;
-	remque(ifm);
+        /* Remove it from the queue */
+        ifqt = ifm->ifq_prev;
+        remque(ifm);
 
-	/* If there are more packets for this session, re-queue them */
-	if (ifm->ifs_next != /* ifm->ifs_prev != */ ifm) {
-		insque(ifm->ifs_next, ifqt);
-		ifs_remque(ifm);
-	}
+        /* If there are more packets for this session, re-queue them */
+        if (ifm->ifs_next != ifm) {
+            insque(ifm->ifs_next, ifqt);
+            ifs_remque(ifm);
+        }
 
-	/* Update so_queued */
-	if (ifm->ifq_so) {
-		if (--ifm->ifq_so->so_queued == 0)
-		   /* If there's no more queued, reset nqueued */
-		   ifm->ifq_so->so_nqueued = 0;
-	}
+        /* Update so_queued */
+        if (ifm->ifq_so && --ifm->ifq_so->so_queued == 0) {
+            /* If there's no more queued, reset nqueued */
+            ifm->ifq_so->so_nqueued = 0;
+        }
 
         m_free(ifm);
 
- out:
-	if (slirp->if_queued)
-	   goto again;
+    }
 
-        slirp->if_queued = requeued;
+    slirp->if_queued = requeued;
 }
-- 
1.7.3.4