[Qemu-devel] [PATCH] Making SLIRP code more 64-bit clean

[Qemu-devel] [PATCH] Making SLIRP code more 64-bit clean

[Scott Pakin]

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

The attached patch corrects a bug in qemu/slirp/tcp_var.h that defines
the seg_next field in struct tcpcb to be 32 bits wide regardless of
32/64-bitness.  seg_next is assigned a pointer value in
qemu/slirp/tcp_subr.c, then cast back to a pointer in qemu/slirp/tcp_input.c
and dereferenced.  That produces a SIGSEGV on my system.

For more information, see the thread "[ 1881532 ] Network access seg faults
KVM on large-memory machine" on the KVM Bugs page on SourceForge
(http://tinyurl.com/2fxfbx).

Regards,
-- Scott

P.S.  Note: This message was sent to both qemu-devel and kvm-devel.

[-- Attachment #2: tcp_seg_next.patch --]
[-- Type: text/x-patch, Size: 403 bytes --]

--- qemu/slirp/tcp_var.h.ORIG   2008-01-28 17:27:09.000000000 -0700
+++ qemu/slirp/tcp_var.h        2008-01-28 17:27:20.000000000 -0700
@@ -40,11 +40,7 @@
 #include "tcpip.h"
 #include "tcp_timer.h"

-#if SIZEOF_CHAR_P == 4
- typedef struct tcpiphdr *tcpiphdrp_32;
-#else
- typedef u_int32_t tcpiphdrp_32;
-#endif
+typedef struct tcpiphdr *tcpiphdrp_32;

 /*
  * Tcp control block, one per tcp; fields:

[Qemu-devel] RE: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Zhang, Xiantao]

Scott Pakin wrote:
> The attached patch corrects a bug in qemu/slirp/tcp_var.h that defines
> the seg_next field in struct tcpcb to be 32 bits wide regardless of
> 32/64-bitness.  seg_next is assigned a pointer value in
> qemu/slirp/tcp_subr.c, then cast back to a pointer in
> qemu/slirp/tcp_input.c and dereferenced.  That produces a SIGSEGV on
> my system. 


I still hit it on IA64 platform with your patch, once configured with
slirp.  
Thanks
Xiantao

[Qemu-devel] Re: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Scott Pakin]

Zhang, Xiantao wrote:
> Scott Pakin wrote:
>> The attached patch corrects a bug in qemu/slirp/tcp_var.h that defines
>> the seg_next field in struct tcpcb to be 32 bits wide regardless of
>> 32/64-bitness.  seg_next is assigned a pointer value in
>> qemu/slirp/tcp_subr.c, then cast back to a pointer in
>> qemu/slirp/tcp_input.c and dereferenced.  That produces a SIGSEGV on
>> my system. 
> 
> 
> I still hit it on IA64 platform with your patch, once configured with
> slirp.  

Okay, here's a more thorough patch that fixes *all* of the "cast from/to
pointer to/from integer of a different size" mistakes that gcc warns
about.  Does it also solve the SIGSEGV problem on IA64?

-- Scott

================== BEGIN tcp_int32_pointer_cast.patch ==================
diff -Naur kvm-60-ORIG/qemu/exec-all.h kvm-60/qemu/exec-all.h
--- kvm-60-ORIG/qemu/exec-all.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/exec-all.h	2008-01-29 19:19:45.000000000 -0700
@@ -169,7 +169,7 @@
  #ifdef USE_DIRECT_JUMP
      uint16_t tb_jmp_offset[4]; /* offset of jump instruction */
  #else
-    uint32_t tb_next[2]; /* address of jump generated code */
+    uintptr_t tb_next[2]; /* address of jump generated code */
  #endif
      /* list of TBs jumping to this one. This is a circular list using
         the two least significant bits of the pointers to tell what is
diff -Naur kvm-60-ORIG/qemu/slirp/ip.h kvm-60/qemu/slirp/ip.h
--- kvm-60-ORIG/qemu/slirp/ip.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/ip.h	2008-01-29 19:13:09.000000000 -0700
@@ -183,23 +183,9 @@

  #define	IP_MSS		576		/* default maximum segment size */

-#ifdef HAVE_SYS_TYPES32_H  /* Overcome some Solaris 2.x junk */
-#include <sys/types32.h>
-#else
-#if SIZEOF_CHAR_P == 4
  typedef caddr_t caddr32_t;
-#else
-typedef u_int32_t caddr32_t;
-#endif
-#endif
-
-#if SIZEOF_CHAR_P == 4
  typedef struct ipq *ipqp_32;
  typedef struct ipasfrag *ipasfragp_32;
-#else
-typedef caddr32_t ipqp_32;
-typedef caddr32_t ipasfragp_32;
-#endif

  /*
   * Overlay for ip header used by other protocols (tcp, udp).
diff -Naur kvm-60-ORIG/qemu/slirp/misc.c kvm-60/qemu/slirp/misc.c
--- kvm-60-ORIG/qemu/slirp/misc.c	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/misc.c	2008-01-29 11:36:15.000000000 -0700
@@ -97,39 +97,6 @@
              our_addr.s_addr = loopback_addr.s_addr;
  }

-#if SIZEOF_CHAR_P == 8
-
-struct quehead_32 {
-	u_int32_t qh_link;
-	u_int32_t qh_rlink;
-};
-
-inline void
-insque_32(a, b)
-	void *a;
-	void *b;
-{
-	register struct quehead_32 *element = (struct quehead_32 *) a;
-	register struct quehead_32 *head = (struct quehead_32 *) b;
-	element->qh_link = head->qh_link;
-	head->qh_link = (u_int32_t)element;
-	element->qh_rlink = (u_int32_t)head;
-	((struct quehead_32 *)(element->qh_link))->qh_rlink
-	= (u_int32_t)element;
-}
-
-inline void
-remque_32(a)
-	void *a;
-{
-	register struct quehead_32 *element = (struct quehead_32 *) a;
-	((struct quehead_32 *)(element->qh_link))->qh_rlink = element->qh_rlink;
-	((struct quehead_32 *)(element->qh_rlink))->qh_link = element->qh_link;
-	element->qh_rlink = 0;
-}
-
-#endif /* SIZEOF_CHAR_P == 8 */
-
  struct quehead {
  	struct quehead *qh_link;
  	struct quehead *qh_rlink;
diff -Naur kvm-60-ORIG/qemu/slirp/slirp.h kvm-60/qemu/slirp/slirp.h
--- kvm-60-ORIG/qemu/slirp/slirp.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/slirp.h	2008-01-29 11:37:19.000000000 -0700
@@ -265,13 +265,8 @@

  void lprint _P((const char *, ...));

-#if SIZEOF_CHAR_P == 4
-# define insque_32 insque
-# define remque_32 remque
-#else
- inline void insque_32 _P((void *, void *));
- inline void remque_32 _P((void *));
-#endif
+#define insque_32 insque
+#define remque_32 remque

  #ifndef _WIN32
  #include <netdb.h>
diff -Naur kvm-60-ORIG/qemu/slirp/tcp_var.h kvm-60/qemu/slirp/tcp_var.h
--- kvm-60-ORIG/qemu/slirp/tcp_var.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/tcp_var.h	2008-01-28 21:12:22.000000000 -0700
@@ -40,11 +40,7 @@
  #include "tcpip.h"
  #include "tcp_timer.h"

-#if SIZEOF_CHAR_P == 4
- typedef struct tcpiphdr *tcpiphdrp_32;
-#else
- typedef u_int32_t tcpiphdrp_32;
-#endif
+typedef struct tcpiphdr *tcpiphdrp_32;

  /*
   * Tcp control block, one per tcp; fields:
@@ -178,11 +174,7 @@
   * port numbers (which are no longer needed once we've located the
   * tcpcb) are overlayed with an mbuf pointer.
   */
-#if SIZEOF_CHAR_P == 4
  typedef struct mbuf *mbufp_32;
-#else
-typedef u_int32_t mbufp_32;
-#endif
  #define REASS_MBUF(ti) (*(mbufp_32 *)&((ti)->ti_t))

  #ifdef LOG_ENABLED
=================== END tcp_int32_pointer_cast.patch ===================

Re: [Qemu-devel] Re: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Blue Swirl]

On 1/30/08, Scott Pakin <pakin@lanl.gov> wrote:
> Zhang, Xiantao wrote:
> > Scott Pakin wrote:
> >> The attached patch corrects a bug in qemu/slirp/tcp_var.h that defines
> >> the seg_next field in struct tcpcb to be 32 bits wide regardless of
> >> 32/64-bitness.  seg_next is assigned a pointer value in
> >> qemu/slirp/tcp_subr.c, then cast back to a pointer in
> >> qemu/slirp/tcp_input.c and dereferenced.  That produces a SIGSEGV on
> >> my system.
> >
> >
> > I still hit it on IA64 platform with your patch, once configured with
> > slirp.
>
> Okay, here's a more thorough patch that fixes *all* of the "cast from/to
> pointer to/from integer of a different size" mistakes that gcc warns
> about.  Does it also solve the SIGSEGV problem on IA64?

The SLIRP code is much, much more subtle than that. Please see this thread:
http://lists.gnu.org/archive/html/qemu-devel/2007-10/msg00542.html

RE: [kvm-ia64-devel] [Qemu-devel] Re: [kvm-devel] [PATCH] MakingSLIRP code more 64-bit clean

[Zhang, Xiantao]

Blue Swirl wrote:
> On 1/30/08, Scott Pakin <pakin@lanl.gov> wrote:
>> Zhang, Xiantao wrote:
>>> Scott Pakin wrote:
>>>> The attached patch corrects a bug in qemu/slirp/tcp_var.h that
>>>> defines the seg_next field in struct tcpcb to be 32 bits wide
>>>> regardless of 32/64-bitness.  seg_next is assigned a pointer value
>>>> in qemu/slirp/tcp_subr.c, then cast back to a pointer in
>>>> qemu/slirp/tcp_input.c and dereferenced.  That produces a SIGSEGV
>>>> on my system.
>>> 
>>> 
>>> I still hit it on IA64 platform with your patch, once configured
>>> with slirp.
>> 
>> Okay, here's a more thorough patch that fixes *all* of the "cast
>> from/to pointer to/from integer of a different size" mistakes that
>> gcc warns about.  Does it also solve the SIGSEGV problem on IA64?
> 
> The SLIRP code is much, much more subtle than that. Please see this
> thread:
> http://lists.gnu.org/archive/html/qemu-devel/2007-10/msg00542.html 

Got it. Thank you! 
Xiantao

[Qemu-devel] RE: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Zhang, Xiantao]

Unfortunately, it can's apply on tip. Could you attach the diff ?
Thanks
Xiantao

[Qemu-devel] Re: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Scott Pakin]

Zhang, Xiantao wrote:
> Unfortunately, it can's apply on tip. Could you attach the diff ?

It works for me:

$ git clone git://git.kernel.org/pub/scm/virt/kvm/kvm-userspace.git
remote: Generating pack...
remote: Done counting 37449 objects.
remote: Deltifying 37449 objects...
remote:  100% (37449/37449) done
Indexing 37449 objects.
remote: Total 37449 (delta 24712), reused 37189 (delta 24506)
  100% (37449/37449) done
Resolving 24712 deltas.
  100% (24712/24712) done
Checking files out...
  100% (948/948) done
$ cd kvm-userspace
$ patch -p1 < ~/tcp_int32_pointer_cast.patch
patching file qemu/exec-all.h
patching file qemu/slirp/ip.h
patching file qemu/slirp/misc.c
patching file qemu/slirp/slirp.h
patching file qemu/slirp/tcp_var.h
$

-- Scott

[Qemu-devel] Re: [kvm-devel] [PATCH] Making SLIRP code more 64-bit clean

[Scott Pakin]

I just noticed that my previous patch hit one of the subtleties that
Blue Swirl warned about.  Changing caddr32_t causes the IP header and
IP header overlay to be different sizes, which essentially breaks
networking altogether.

I humbly offer the following patch, which fixes only the "easy" 32/64-bit
bugs but leaves the tricky 32/64-bit bugs in the IP header processing
intact for someone abler than I to fix.

-- Scott

============= BEGIN tcp_int32_pointer_cast_no_caddr.patch ==============
diff -Naur kvm-60-ORIG/qemu/exec-all.h kvm-60/qemu/exec-all.h
--- kvm-60-ORIG/qemu/exec-all.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/exec-all.h	2008-01-31 17:36:34.000000000 -0700
@@ -169,7 +169,7 @@
  #ifdef USE_DIRECT_JUMP
      uint16_t tb_jmp_offset[4]; /* offset of jump instruction */
  #else
-    uint32_t tb_next[2]; /* address of jump generated code */
+    uintptr_t tb_next[2]; /* address of jump generated code */
  #endif
      /* list of TBs jumping to this one. This is a circular list using
         the two least significant bits of the pointers to tell what is
diff -Naur kvm-60-ORIG/qemu/slirp/ip.h kvm-60/qemu/slirp/ip.h
--- kvm-60-ORIG/qemu/slirp/ip.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/ip.h	2008-01-31 17:29:28.000000000 -0700
@@ -193,13 +193,8 @@
  #endif
  #endif

-#if SIZEOF_CHAR_P == 4
  typedef struct ipq *ipqp_32;
  typedef struct ipasfrag *ipasfragp_32;
-#else
-typedef caddr32_t ipqp_32;
-typedef caddr32_t ipasfragp_32;
-#endif

  /*
   * Overlay for ip header used by other protocols (tcp, udp).
diff -Naur kvm-60-ORIG/qemu/slirp/misc.c kvm-60/qemu/slirp/misc.c
--- kvm-60-ORIG/qemu/slirp/misc.c	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/misc.c	2008-01-31 17:30:14.000000000 -0700
@@ -97,39 +97,6 @@
              our_addr.s_addr = loopback_addr.s_addr;
  }

-#if SIZEOF_CHAR_P == 8
-
-struct quehead_32 {
-	u_int32_t qh_link;
-	u_int32_t qh_rlink;
-};
-
-inline void
-insque_32(a, b)
-	void *a;
-	void *b;
-{
-	register struct quehead_32 *element = (struct quehead_32 *) a;
-	register struct quehead_32 *head = (struct quehead_32 *) b;
-	element->qh_link = head->qh_link;
-	head->qh_link = (u_int32_t)element;
-	element->qh_rlink = (u_int32_t)head;
-	((struct quehead_32 *)(element->qh_link))->qh_rlink
-	= (u_int32_t)element;
-}
-
-inline void
-remque_32(a)
-	void *a;
-{
-	register struct quehead_32 *element = (struct quehead_32 *) a;
-	((struct quehead_32 *)(element->qh_link))->qh_rlink = element->qh_rlink;
-	((struct quehead_32 *)(element->qh_rlink))->qh_link = element->qh_link;
-	element->qh_rlink = 0;
-}
-
-#endif /* SIZEOF_CHAR_P == 8 */
-
  struct quehead {
  	struct quehead *qh_link;
  	struct quehead *qh_rlink;
diff -Naur kvm-60-ORIG/qemu/slirp/slirp.h kvm-60/qemu/slirp/slirp.h
--- kvm-60-ORIG/qemu/slirp/slirp.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/slirp.h	2008-01-31 17:30:58.000000000 -0700
@@ -265,13 +265,8 @@

  void lprint _P((const char *, ...));

-#if SIZEOF_CHAR_P == 4
-# define insque_32 insque
-# define remque_32 remque
-#else
- inline void insque_32 _P((void *, void *));
- inline void remque_32 _P((void *));
-#endif
+#define insque_32 insque
+#define remque_32 remque

  #ifndef _WIN32
  #include <netdb.h>
diff -Naur kvm-60-ORIG/qemu/slirp/tcp_var.h kvm-60/qemu/slirp/tcp_var.h
--- kvm-60-ORIG/qemu/slirp/tcp_var.h	2008-01-20 05:35:04.000000000 -0700
+++ kvm-60/qemu/slirp/tcp_var.h	2008-01-31 17:35:03.000000000 -0700
@@ -40,11 +40,7 @@
  #include "tcpip.h"
  #include "tcp_timer.h"

-#if SIZEOF_CHAR_P == 4
- typedef struct tcpiphdr *tcpiphdrp_32;
-#else
- typedef u_int32_t tcpiphdrp_32;
-#endif
+typedef struct tcpiphdr *tcpiphdrp_32;

  /*
   * Tcp control block, one per tcp; fields:
@@ -178,11 +174,7 @@
   * port numbers (which are no longer needed once we've located the
   * tcpcb) are overlayed with an mbuf pointer.
   */
-#if SIZEOF_CHAR_P == 4
  typedef struct mbuf *mbufp_32;
-#else
-typedef u_int32_t mbufp_32;
-#endif
  #define REASS_MBUF(ti) (*(mbufp_32 *)&((ti)->ti_t))

  #ifdef LOG_ENABLED
============== END tcp_int32_pointer_cast_no_caddr.patch ===============

[Qemu-devel] RE: [kvm-ia64-devel] [kvm-devel] [PATCH] Making SLIRP code more64-bit clean

[Zhang, Xiantao]

Scott Pakin wrote:
> Zhang, Xiantao wrote:
>> Scott Pakin wrote:
>>> The attached patch corrects a bug in qemu/slirp/tcp_var.h that
>>> defines the seg_next field in struct tcpcb to be 32 bits wide
>>> regardless of 32/64-bitness.  seg_next is assigned a pointer value
>>> in qemu/slirp/tcp_subr.c, then cast back to a pointer in
>>> qemu/slirp/tcp_input.c and dereferenced.  That produces a SIGSEGV on
>>> my system.
>> 
>> 
>> I still hit it on IA64 platform with your patch, once configured with
>> slirp.

Scott
	With the enhanced patch, IA64 guests works well. Great!!  If
this fix be picked up, we can remove the configure option which excludes
slirp compile for kvm/ia64. 
Thanks!
Xiantao