Making SLIRP code more 64-bit clean

PATCH In review Scott Pakin 3 participants 9 messages Jan 29, 2008 – Feb 1, 2008 6 messages not in the archive KVM

Share & tools

Scott Pakin

author

2008-01-29 17:11 UTC #0 lore raw

PATCH 1/1 Making SLIRP code more 64-bit clean

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.

slirp/tcp_var.h 2008-01-28 17:27:20.000000000 -0700 renamed from slirp/tcp_var.h.ORIG 2008-01-28 17:27:09.000000000 -0700 +1 -5

@@ -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:

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

Zhang, Xiantao

2008-01-30 04:27 UTC #2 lore raw

Quoted 6 lines · Scott Pakin

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

A message that is not in the archive (sent off-list or not yet received): 42DFA526FC41B1429CE7279EF83C6BDCD31A15-wq7ZOvIWXbMAbVU2wMM1CrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Scott Pakin

author

2008-01-30 16:42 UTC #4 lore raw

Quoted 11 lines · Xiantao

Quoted 6 lines · Scott Pakin
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 ==================

qemu/exec-all.h +1 -1

diff -Naur kvm-60-ORIG/qemu/exec-all.h kvm-60/qemu/exec-all.h
@@ -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

qemu/slirp/ip.h +0 ~~-14~~

diff -Naur kvm-60-ORIG/qemu/slirp/ip.h kvm-60/qemu/slirp/ip.h
@@ -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).

qemu/slirp/misc.c +0 ~~-33~~

diff -Naur kvm-60-ORIG/qemu/slirp/misc.c kvm-60/qemu/slirp/misc.c
@@ -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;

qemu/slirp/slirp.h +2 -7

diff -Naur kvm-60-ORIG/qemu/slirp/slirp.h kvm-60/qemu/slirp/slirp.h
@@ -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>

qemu/slirp/tcp_var.h +1 -9

diff -Naur kvm-60-ORIG/qemu/slirp/tcp_var.h kvm-60/qemu/slirp/tcp_var.h
@@ -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 ===================

A message that is not in the archive (sent off-list or not yet received): [email protected]

Blue Swirl

2008-01-30 17:10 UTC #6 lore raw

Quoted 10 lines · Scott Pakin

Quoted 9 lines · Xiantao
Quoted 6 lines · Scott Pakin
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.

Quoted 6 lines

Quoted 2 lines
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

A message that is not in the archive (sent off-list or not yet received): f43fc5580801300910w583f39br18d2c6e60e2370a3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Zhang, Xiantao

2008-02-01 01:26 UTC #8 lore raw

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

Quoted 14 lines · Blue Swirl

Quoted 13 lines · Scott Pakin
Quoted 11 lines · Xiantao
Quoted 6 lines · Scott Pakin
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.

Quoted 7 lines

Quoted 3 lines
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

Zhang, Xiantao

2008-01-31 08:24 UTC #9 lore raw

In reply to a message not in the archive

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

A message that is not in the archive (sent off-list or not yet received): 42DFA526FC41B1429CE7279EF83C6BDCD31F4E-wq7ZOvIWXbMAbVU2wMM1CrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Scott Pakin

author

2008-01-31 18:38 UTC #11 lore raw

Quoted 1 line · Xiantao

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Scott Pakin

author

2008-02-01 01:37 UTC #13 lore raw

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 ==============

qemu/exec-all.h +1 -1

diff -Naur kvm-60-ORIG/qemu/exec-all.h kvm-60/qemu/exec-all.h
@@ -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

qemu/slirp/ip.h +0 -5

diff -Naur kvm-60-ORIG/qemu/slirp/ip.h kvm-60/qemu/slirp/ip.h
@@ -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).

qemu/slirp/misc.c +0 ~~-33~~

diff -Naur kvm-60-ORIG/qemu/slirp/misc.c kvm-60/qemu/slirp/misc.c
@@ -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;

qemu/slirp/slirp.h +2 -7

diff -Naur kvm-60-ORIG/qemu/slirp/slirp.h kvm-60/qemu/slirp/slirp.h
@@ -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>

qemu/slirp/tcp_var.h +1 -9

diff -Naur kvm-60-ORIG/qemu/slirp/tcp_var.h kvm-60/qemu/slirp/tcp_var.h
@@ -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 ===============

Zhang, Xiantao

2008-02-01 02:44 UTC #14 lore raw

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

In reply to a message not in the archive

Quoted 10 lines · Scott Pakin

Quoted 9 lines · Xiantao
Quoted 6 lines · Scott Pakin
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.

Quoted 2 lines

Quoted 2 lines
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