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

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

[Jan Kiszka]

Well, this requeuing bug seems to have a long breath. Previous attempts
to fix it (mine included) neglected the fact that we need to walk the
queue of pending packets, not just restart from the beginning after a
requeue. This version should get it Right(TM).

This also comes with a fix for resource cleanups on slirp shutdown. At
least valgrind is happy now.

Changes in v3:
 - Finally fixed Stefan's scenario by avoiding the recursion of if_start
 - Fixed further corner cases in walking the packet queues

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

Jan Kiszka (5):
  slirp: Keep next_m always valid
  slirp: Prevent recursion of if_start
  slirp: Fix queue walking in if_start
  slirp: Remove unneeded if_queued
  slirp: Cleanup resources on instance removal

 slirp/if.c       |   86 ++++++++++++++++++++++++++++++++++-------------------
 slirp/ip_icmp.c  |    7 ++++
 slirp/ip_icmp.h  |    1 +
 slirp/ip_input.c |    7 ++++
 slirp/mbuf.c     |   21 +++++++++++++
 slirp/mbuf.h     |    1 +
 slirp/slirp.c    |   10 ++----
 slirp/slirp.h    |    4 ++-
 slirp/tcp_subr.c |    7 ++++
 slirp/udp.c      |    8 +++++
 slirp/udp.h      |    1 +
 11 files changed, 115 insertions(+), 38 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH v3 1/5] slirp: Keep next_m always valid

[Jan Kiszka]

Make sure that next_m always points to a packet if batchq is non-empty.
This will simplify walking the queues in if_start.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 33f08e1..14fdef1 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -96,8 +96,13 @@ if_output(struct socket *so, struct mbuf *ifm)
 			ifs_insque(ifm, ifq->ifs_prev);
 			goto diddit;
 		}
-	} else
+        } else {
 		ifq = slirp->if_batchq.ifq_prev;
+                /* Set next_m if the queue was empty so far */
+                if (slirp->next_m == &slirp->if_batchq) {
+                    slirp->next_m = ifm;
+                }
+        }
 
 	/* Create a new doubly linked list for this session */
 	ifm->ifq_so = so;
@@ -170,13 +175,8 @@ void if_start(Slirp *slirp)
         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;
-            }
-
+            /* Nothing on fastq, pick up from batchq via next_m */
+            ifm = slirp->next_m;
             from_batchq = true;
         }
 
@@ -202,6 +202,12 @@ void if_start(Slirp *slirp)
         if (ifm->ifs_next != ifm) {
             insque(ifm->ifs_next, ifqt);
             ifs_remque(ifm);
+            /* Set next_m if the session packet is now the only one on
+             * batchq */
+            if (ifqt == &slirp->if_batchq &&
+                slirp->next_m == &slirp->if_batchq) {
+                slirp->next_m = ifm->ifs_next;
+            }
         }
 
         /* Update so_queued */
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 2/5] slirp: Prevent recursion of if_start

[Jan Kiszka]

if_start can be called recursively via if_encap. Avoid this as our
scheme of dequeuing packets is not compatible with this.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c    |   11 ++++++++++-
 slirp/slirp.h |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index 14fdef1..f7aebe9 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -163,10 +163,17 @@ void if_start(Slirp *slirp)
 
     DEBUG_CALL("if_start");
 
+    if (slirp->if_start_busy) {
+        return;
+    }
+    slirp->if_start_busy = true;
+
     while (slirp->if_queued) {
         /* check if we can really output */
-        if (!slirp_can_output(slirp->opaque))
+        if (!slirp_can_output(slirp->opaque)) {
+            slirp->if_start_busy = false;
             return;
+        }
 
         /*
          * See which queue to get next packet from
@@ -221,4 +228,6 @@ void if_start(Slirp *slirp)
     }
 
     slirp->if_queued = requeued;
+
+    slirp->if_start_busy = false;
 }
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 28a5c03..416d44a 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -239,6 +239,7 @@ struct Slirp {
     struct mbuf if_fastq;   /* fast queue (for interactive data) */
     struct mbuf if_batchq;  /* queue for non-interactive data */
     struct mbuf *next_m;    /* pointer to next mbuf to output */
+    bool if_start_busy;     /* avoid if_start recursion */
 
     /* ip states */
     struct ipq ipq;         /* ip reass. queue */
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 3/5] slirp: Fix queue walking in if_start

[Jan Kiszka]

Another attempt to get this right: We need to carefully walk both the
fastq and the batchq in if_start while trying to send packets to
possibly not yet resolved hosts on the virtual network.

So far we just requeued a delayed packet where it was and then started
walking the queues from the top again - that couldn't work. Now we pre-
calculate the next packet in the queue so that the current one can
safely be removed if it was sent successfully. We also need to take into
account that the next packet can be from the same session if the current
one was sent and there are no other sessions.

CC: Fabien Chouteau <chouteau@adacore.com>
CC: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
Tested-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c |   60 +++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index f7aebe9..f6e848a 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -158,8 +158,8 @@ 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;
+    bool from_batchq, next_from_batchq;
+    struct mbuf *ifm, *ifm_next, *ifqt;
 
     DEBUG_CALL("if_start");
 
@@ -168,23 +168,36 @@ void if_start(Slirp *slirp)
     }
     slirp->if_start_busy = true;
 
-    while (slirp->if_queued) {
+    if (slirp->if_fastq.ifq_next != &slirp->if_fastq) {
+        ifm_next = slirp->if_fastq.ifq_next;
+        next_from_batchq = false;
+    } else if (slirp->next_m != &slirp->if_batchq) {
+        /* Nothing on fastq, pick up from batchq via next_m */
+        ifm_next = slirp->next_m;
+        next_from_batchq = true;
+    } else {
+        ifm_next = NULL;
+    }
+
+    while (ifm_next) {
         /* check if we can really output */
         if (!slirp_can_output(slirp->opaque)) {
             slirp->if_start_busy = false;
             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, pick up from batchq via next_m */
-            ifm = slirp->next_m;
-            from_batchq = true;
+        ifm = ifm_next;
+        from_batchq = next_from_batchq;
+
+        ifm_next = ifm->ifq_next;
+        if (ifm_next == &slirp->if_fastq) {
+            /* No more packets in fastq, switch to batchq */
+            ifm_next = slirp->next_m;
+            next_from_batchq = true;
+        }
+        if (ifm_next == &slirp->if_batchq) {
+            /* end of batchq */
+            ifm_next = NULL;
         }
 
         slirp->if_queued--;
@@ -196,7 +209,7 @@ void if_start(Slirp *slirp)
             continue;
         }
 
-        if (from_batchq) {
+        if (ifm == slirp->next_m) {
             /* Set which packet to send on next iteration */
             slirp->next_m = ifm->ifq_next;
         }
@@ -207,13 +220,19 @@ void if_start(Slirp *slirp)
 
         /* If there are more packets for this session, re-queue them */
         if (ifm->ifs_next != ifm) {
-            insque(ifm->ifs_next, ifqt);
+            struct mbuf *next = ifm->ifs_next;
+
+            insque(next, ifqt);
             ifs_remque(ifm);
-            /* Set next_m if the session packet is now the only one on
-             * batchq */
-            if (ifqt == &slirp->if_batchq &&
-                slirp->next_m == &slirp->if_batchq) {
-                slirp->next_m = ifm->ifs_next;
+
+            if (!from_batchq) {
+                /* Next packet in fastq is from the same session */
+                ifm_next = next;
+                next_from_batchq = false;
+            } else if (slirp->next_m == &slirp->if_batchq) {
+                /* Set next_m and ifm_next if the session packet is now the
+                 * only one on batchq */
+                slirp->next_m = ifm_next = next;
             }
         }
 
@@ -224,7 +243,6 @@ void if_start(Slirp *slirp)
         }
 
         m_free(ifm);
-
     }
 
     slirp->if_queued = requeued;
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 4/5] slirp: Remove unneeded if_queued

[Jan Kiszka]

There is now a trivial check on entry of if_start for pending packets,
so we can drop the additional tracking via if_queued.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/if.c    |   11 +----------
 slirp/slirp.c |    7 +------
 slirp/slirp.h |    1 -
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/slirp/if.c b/slirp/if.c
index f6e848a..096cf6f 100644
--- a/slirp/if.c
+++ b/slirp/if.c
@@ -110,8 +110,6 @@ if_output(struct socket *so, struct mbuf *ifm)
 	insque(ifm, ifq);
 
 diddit:
-	slirp->if_queued++;
-
 	if (so) {
 		/* Update *_queued */
 		so->so_queued++;
@@ -157,7 +155,6 @@ diddit:
 void if_start(Slirp *slirp)
 {
     uint64_t now = qemu_get_clock_ns(rt_clock);
-    int requeued = 0;
     bool from_batchq, next_from_batchq;
     struct mbuf *ifm, *ifm_next, *ifqt;
 
@@ -182,8 +179,7 @@ void if_start(Slirp *slirp)
     while (ifm_next) {
         /* check if we can really output */
         if (!slirp_can_output(slirp->opaque)) {
-            slirp->if_start_busy = false;
-            return;
+            break;
         }
 
         ifm = ifm_next;
@@ -200,12 +196,9 @@ void if_start(Slirp *slirp)
             ifm_next = NULL;
         }
 
-        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++;
             continue;
         }
 
@@ -245,7 +238,5 @@ void if_start(Slirp *slirp)
         m_free(ifm);
     }
 
-    slirp->if_queued = requeued;
-
     slirp->if_start_busy = false;
 }
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 19d69eb..bcffc34 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -581,12 +581,7 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
                 }
 	}
 
-	/*
-	 * See if we can start outputting
-	 */
-	if (slirp->if_queued) {
-	    if_start(slirp);
-	}
+        if_start(slirp);
     }
 
 	/* clear global file descriptor sets.
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 416d44a..cbe8a3c 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -235,7 +235,6 @@ struct Slirp {
     int mbuf_alloced;
 
     /* if states */
-    int if_queued;          /* number of packets queued so far */
     struct mbuf if_fastq;   /* fast queue (for interactive data) */
     struct mbuf if_batchq;  /* queue for non-interactive data */
     struct mbuf *next_m;    /* pointer to next mbuf to output */
-- 
1.7.3.4

[Qemu-devel] [PATCH v3 5/5] slirp: Cleanup resources on instance removal

[Jan Kiszka]

Close & free sockets when shutting down a slirp instance, also release
all buffers.

CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 slirp/ip_icmp.c  |    7 +++++++
 slirp/ip_icmp.h  |    1 +
 slirp/ip_input.c |    7 +++++++
 slirp/mbuf.c     |   21 +++++++++++++++++++++
 slirp/mbuf.h     |    1 +
 slirp/slirp.c    |    3 +++
 slirp/slirp.h    |    2 ++
 slirp/tcp_subr.c |    7 +++++++
 slirp/udp.c      |    8 ++++++++
 slirp/udp.h      |    1 +
 10 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 5dbf21d..d571fd0 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -66,6 +66,13 @@ void icmp_init(Slirp *slirp)
     slirp->icmp_last_so = &slirp->icmp;
 }
 
+void icmp_cleanup(Slirp *slirp)
+{
+    while (slirp->icmp.so_next != &slirp->icmp) {
+        icmp_detach(slirp->icmp.so_next);
+    }
+}
+
 static int icmp_send(struct socket *so, struct mbuf *m, int hlen)
 {
     struct ip *ip = mtod(m, struct ip *);
diff --git a/slirp/ip_icmp.h b/slirp/ip_icmp.h
index b3da1f2..1a1af91 100644
--- a/slirp/ip_icmp.h
+++ b/slirp/ip_icmp.h
@@ -154,6 +154,7 @@ struct icmp {
 	(type) == ICMP_MASKREQ || (type) == ICMP_MASKREPLY)
 
 void icmp_init(Slirp *slirp);
+void icmp_cleanup(Slirp *slirp);
 void icmp_input(struct mbuf *, int);
 void icmp_error(struct mbuf *msrc, u_char type, u_char code, int minsize,
                 const char *message);
diff --git a/slirp/ip_input.c b/slirp/ip_input.c
index c7b3eb4..ce24faf 100644
--- a/slirp/ip_input.c
+++ b/slirp/ip_input.c
@@ -61,6 +61,13 @@ ip_init(Slirp *slirp)
     icmp_init(slirp);
 }
 
+void ip_cleanup(Slirp *slirp)
+{
+    udp_cleanup(slirp);
+    tcp_cleanup(slirp);
+    icmp_cleanup(slirp);
+}
+
 /*
  * Ip input routine.  Checksum and byte swap header.  If fragmented
  * try to reassemble.  Process options.  Pass to next level.
diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index c699c75..4fefb04 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -32,6 +32,27 @@ m_init(Slirp *slirp)
     slirp->m_usedlist.m_next = slirp->m_usedlist.m_prev = &slirp->m_usedlist;
 }
 
+void m_cleanup(Slirp *slirp)
+{
+    struct mbuf *m, *next;
+
+    m = slirp->m_usedlist.m_next;
+    while (m != &slirp->m_usedlist) {
+        next = m->m_next;
+        if (m->m_flags & M_EXT) {
+            free(m->m_ext);
+        }
+        free(m);
+        m = next;
+    }
+    m = slirp->m_freelist.m_next;
+    while (m != &slirp->m_freelist) {
+        next = m->m_next;
+        free(m);
+        m = next;
+    }
+}
+
 /*
  * Get an mbuf from the free list, if there are none
  * malloc one
diff --git a/slirp/mbuf.h b/slirp/mbuf.h
index 8d7951f..3f3ab09 100644
--- a/slirp/mbuf.h
+++ b/slirp/mbuf.h
@@ -116,6 +116,7 @@ struct mbuf {
 					 * it rather than putting it on the free list */
 
 void m_init(Slirp *);
+void m_cleanup(Slirp *slirp);
 struct mbuf * m_get(Slirp *);
 void m_free(struct mbuf *);
 void m_cat(register struct mbuf *, register struct mbuf *);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index bcffc34..1502830 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -246,6 +246,9 @@ void slirp_cleanup(Slirp *slirp)
 
     unregister_savevm(NULL, "slirp", slirp);
 
+    ip_cleanup(slirp);
+    m_cleanup(slirp);
+
     g_free(slirp->tftp_prefix);
     g_free(slirp->bootp_filename);
     g_free(slirp);
diff --git a/slirp/slirp.h b/slirp/slirp.h
index cbe8a3c..5033ee3 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -315,6 +315,7 @@ void if_output(struct socket *, struct mbuf *);
 
 /* ip_input.c */
 void ip_init(Slirp *);
+void ip_cleanup(Slirp *);
 void ip_input(struct mbuf *);
 void ip_slowtimo(Slirp *);
 void ip_stripoptions(register struct mbuf *, struct mbuf *);
@@ -332,6 +333,7 @@ void tcp_setpersist(register struct tcpcb *);
 
 /* tcp_subr.c */
 void tcp_init(Slirp *);
+void tcp_cleanup(Slirp *);
 void tcp_template(struct tcpcb *);
 void tcp_respond(struct tcpcb *, register struct tcpiphdr *, register struct mbuf *, tcp_seq, tcp_seq, int);
 struct tcpcb * tcp_newtcpcb(struct socket *);
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 143a238..6f6585a 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -55,6 +55,13 @@ tcp_init(Slirp *slirp)
     slirp->tcp_last_so = &slirp->tcb;
 }
 
+void tcp_cleanup(Slirp *slirp)
+{
+    while (slirp->tcb.so_next != &slirp->tcb) {
+        tcp_close(sototcpcb(slirp->tcb.so_next));
+    }
+}
+
 /*
  * Create template to be used to send tcp packets on a connection.
  * Call after host entry created, fills
diff --git a/slirp/udp.c b/slirp/udp.c
index 5b060f3..ced5096 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -49,6 +49,14 @@ udp_init(Slirp *slirp)
     slirp->udb.so_next = slirp->udb.so_prev = &slirp->udb;
     slirp->udp_last_so = &slirp->udb;
 }
+
+void udp_cleanup(Slirp *slirp)
+{
+    while (slirp->udb.so_next != &slirp->udb) {
+        udp_detach(slirp->udb.so_next);
+    }
+}
+
 /* m->m_data  points at ip packet header
  * m->m_len   length ip packet
  * ip->ip_len length data (IPDU)
diff --git a/slirp/udp.h b/slirp/udp.h
index 9b5c3cf..9bf31fe 100644
--- a/slirp/udp.h
+++ b/slirp/udp.h
@@ -74,6 +74,7 @@ struct udpiphdr {
 struct mbuf;
 
 void udp_init(Slirp *);
+void udp_cleanup(Slirp *);
 void udp_input(register struct mbuf *, int);
 int udp_output(struct socket *, struct mbuf *, struct sockaddr_in *);
 int udp_attach(struct socket *);
-- 
1.7.3.4