From: Zhang, X. <xia...@in...> - 2008-01-30 04:28:10
|
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.=20 I still hit it on IA64 platform with your patch, once configured with slirp. =20 Thanks Xiantao |
From: Zhang, X. <xia...@in...> - 2008-01-31 08:28:18
|
Unfortunately, it can's apply on tip. Could you attach the diff ? Thanks Xiantao |
From: Scott P. <pa...@la...> - 2008-01-31 18:38:36
|
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 |
From: Scott P. <pa...@la...> - 2008-02-01 01:37:29
|
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 =============== |
From: Scott P. <pa...@la...> - 2008-01-30 16:48:47
|
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 =================== |
From: Blue S. <bla...@gm...> - 2008-01-30 17:10:41
|
On 1/30/08, Scott Pakin <pa...@la...> 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 |
From: Zhang, X. <xia...@in...> - 2008-02-01 01:39:23
|
Blue Swirl wrote: > On 1/30/08, Scott Pakin <pa...@la...> 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. >>>=20 >>>=20 >>> I still hit it on IA64 platform with your patch, once configured >>> with slirp. >>=20 >> 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? >=20 > 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=20 Got it. Thank you!=20 Xiantao |
From: Zhang, X. <xia...@in...> - 2008-02-01 02:44:17
|
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. >>=20 >>=20 >> 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.=20 Thanks! Xiantao |