In qemu-0.8.0.20060327, there are three problems with sending large
packets from guest to host:

 (1) the code in slirp's ip_reass() reads a next pointer out an mbuf
after freeing it via m_cat().

 (2) the code in slirp's m_inc() calls realloc() on a large mbuf, but
fails to adjust m_data to point to the new allocation (see
http://lists.gnu.org/archive/html/qemu-devel/2005-05/msg00228.html).

 (3) there are many places within ip_input(), ip_reass(),
udp_input(), etc., that treat ip_len and ip_off as though they were
declared unsigned, when in fact they have been declared signed.

Patches fixing these problems are attached. I hope they can be
applied.  Please let me know what I can do to make the patches more
likely to be accepted.

Thanks,

   -Ken
diff -BurN qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c 
qemu-snapshot-2006-03-27_23/slirp/ip_input.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c   2004-04-22 
00:10:47.000000000 +0000
+++ qemu-snapshot-2006-03-27_23/slirp/ip_input.c        2006-04-06 
06:02:52.000000000 +0000
@@ -344,8 +344,8 @@
        while (q != (struct ipasfrag *)fp) {
          struct mbuf *t;
          t = dtom(q);
-         m_cat(m, t);
          q = (struct ipasfrag *) q->ipf_next;
+         m_cat(m, t);
        }
 
        /*

diff -BurN qemu-snapshot-2006-03-27_23.orig/slirp/mbuf.c 
qemu-snapshot-2006-03-27_23/slirp/mbuf.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/mbuf.c       2004-04-22 
00:10:47.000000000 +0000
+++ qemu-snapshot-2006-03-27_23/slirp/mbuf.c    2006-04-05 13:03:03.000000000 
+0000
@@ -146,18 +146,19 @@
         struct mbuf *m;
         int size;
 {
+       int datasize;
+
        /* some compiles throw up on gotos.  This one we can fake. */
         if(m->m_size>size) return;
 
         if (m->m_flags & M_EXT) {
-         /* datasize = m->m_data - m->m_ext; */
+         datasize = m->m_data - m->m_ext;
          m->m_ext = (char *)realloc(m->m_ext,size);
 /*             if (m->m_ext == NULL)
  *                     return (struct mbuf *)NULL;
  */            
-         /* m->m_data = m->m_ext + datasize; */
+         m->m_data = m->m_ext + datasize;
         } else {
-         int datasize;
          char *dat;
          datasize = m->m_data - m->m_dat;
          dat = (char *)malloc(size);

diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/ip.h 
qemu-snapshot-2006-03-27_23/slirp/ip.h
--- qemu-snapshot-2006-03-27_23.orig/slirp/ip.h 2004-04-21 17:10:47.000000000 
-0700
+++ qemu-snapshot-2006-03-27_23/slirp/ip.h      2006-04-06 00:28:49.000000000 
-0700
@@ -79,6 +79,11 @@
  * We declare ip_len and ip_off to be short, rather than u_short
  * pragmatically since otherwise unsigned comparisons can result
  * against negative integers quite easily, and fail in subtle ways.
+ *
+ * The only problem with the above theory is that these quantities
+ * are in fact unsigned, and sorting fragments by a signed version
+ * of ip_off doesn't work very well, nor does length checks on
+ * ip packets with a signed version of their length!
  */
 struct ip {
 #ifdef WORDS_BIGENDIAN
@@ -101,6 +106,9 @@
        struct  in_addr ip_src,ip_dst;  /* source and dest address */
 };
 
+#define IP_OFF(ip) (*(u_int16_t *)&((ip)->ip_off))
+#define IP_LEN(ip) (*(u_int16_t *)&((ip)->ip_len))
+
 #define        IP_MAXPACKET    65535           /* maximum packet size */
 
 /*
diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c 
qemu-snapshot-2006-03-27_23/slirp/ip_input.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c   2004-04-21 
17:10:47.000000000 -0700
+++ qemu-snapshot-2006-03-27_23/slirp/ip_input.c        2006-04-06 
00:32:19.000000000 -0700
@@ -111,7 +111,7 @@
         * Convert fields to host representation.
         */
        NTOHS(ip->ip_len);
-       if (ip->ip_len < hlen) {
+       if (IP_LEN(ip) < hlen) {
                ipstat.ips_badlen++;
                goto bad;
        }
@@ -124,13 +124,13 @@
         * Trim mbufs if longer than we expect.
         * Drop packet if shorter than we expect.
         */
-       if (m->m_len < ip->ip_len) {
+       if (m->m_len < IP_LEN(ip)) {
                ipstat.ips_tooshort++;
                goto bad;
        }
        /* Should drop packet if mbuf too long? hmmm... */
-       if (m->m_len > ip->ip_len)
-          m_adj(m, ip->ip_len - m->m_len);
+       if (m->m_len > IP_LEN(ip))
+          m_adj(m, IP_LEN(ip) - m->m_len);
 
        /* check ip_ttl for a correct ICMP reply */
        if(ip->ip_ttl==0 || ip->ip_ttl==1) {
@@ -191,7 +191,7 @@
                 * or if this is not the first fragment,
                 * attempt reassembly; if it succeeds, proceed.
                 */
-               if (((struct ipasfrag *)ip)->ipf_mff & 1 || ip->ip_off) {
+               if (((struct ipasfrag *)ip)->ipf_mff & 1 || IP_OFF(ip)) {
                        ipstat.ips_fragments++;
                        ip = ip_reass((struct ipasfrag *)ip, fp);
                        if (ip == 0)
@@ -281,7 +281,7 @@
         */
        for (q = (struct ipasfrag *)fp->ipq_next; q != (struct ipasfrag *)fp;
            q = (struct ipasfrag *)q->ipf_next)
-               if (q->ip_off > ip->ip_off)
+               if (IP_OFF(q) > IP_OFF(ip))
                        break;
 
        /*
@@ -290,10 +290,10 @@
         * segment.  If it provides all of our data, drop us.
         */
        if (q->ipf_prev != (ipasfragp_32)fp) {
-               i = ((struct ipasfrag *)(q->ipf_prev))->ip_off +
-                 ((struct ipasfrag *)(q->ipf_prev))->ip_len - ip->ip_off;
+               i = IP_OFF((struct ipasfrag *)(q->ipf_prev)) +
+                 IP_LEN((struct ipasfrag *)(q->ipf_prev)) - IP_OFF(ip);
                if (i > 0) {
-                       if (i >= ip->ip_len)
+                       if (i >= IP_LEN(ip))
                                goto dropfrag;
                        m_adj(dtom(ip), i);
                        ip->ip_off += i;
@@ -305,9 +305,9 @@
         * While we overlap succeeding segments trim them or,
         * if they are completely covered, dequeue them.
         */
-       while (q != (struct ipasfrag *)fp && ip->ip_off + ip->ip_len > 
q->ip_off) {
-               i = (ip->ip_off + ip->ip_len) - q->ip_off;
-               if (i < q->ip_len) {
+       while (q != (struct ipasfrag *)fp && IP_OFF(ip) + IP_LEN(ip) > 
IP_OFF(q)) {
+               i = (IP_OFF(ip) + IP_LEN(ip)) - IP_OFF(q);
+               if (i < IP_LEN(q)) {
                        q->ip_len -= i;
                        q->ip_off += i;
                        m_adj(dtom(q), i);
@@ -327,9 +327,9 @@
        next = 0;
        for (q = (struct ipasfrag *) fp->ipq_next; q != (struct ipasfrag *)fp;
             q = (struct ipasfrag *) q->ipf_next) {
-               if (q->ip_off != next)
+               if (IP_OFF(q) != next)
                        return (0);
-               next += q->ip_len;
+               next += IP_LEN(q);
        }
        if (((struct ipasfrag *)(q->ipf_prev))->ipf_mff & 1)
                return (0);
diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/udp.c 
qemu-snapshot-2006-03-27_23/slirp/udp.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/udp.c        2006-04-06 
00:24:30.000000000 -0700
+++ qemu-snapshot-2006-03-27_23/slirp/udp.c     2006-04-06 00:32:55.000000000 
-0700
@@ -111,12 +111,12 @@
         */
        len = ntohs((u_int16_t)uh->uh_ulen);
 
-       if (ip->ip_len != len) {
-               if (len > ip->ip_len) {
+       if (IP_LEN(ip) != len) {
+               if (len > IP_LEN(ip)) {
                        udpstat.udps_badlen++;
                        goto bad;
                }
-               m_adj(m, len - ip->ip_len);
+               m_adj(m, len - IP_LEN(ip));
                ip->ip_len = len;
        }
        

_______________________________________________
Qemu-devel mailing list
Qemu-devel@nongnu.org
http://lists.nongnu.org/mailman/listinfo/qemu-devel

Reply via email to