Bug#764476: vde2: Segfaults due to race condition between find_in_hash() and hash_gc()

[Bas van Sisseren]

Package: vde2
Version: 2.3.2-4.2
Severity: important
Tags: upstream patch

Hello,

We're using the vde2-switch as network switch for several qemu-processes. 
Lately we
noticed a lot of segfaults from the vde2-switch process. We were able to 
trigger the
segfaults more often when we generate a lot of random mac-addresses on the 
network,
combined with a lot of traffic.

After diving into the code, I noticed that the hash_gc() method is called from 
the
SIGALRM signal handler, which could happen at the same time as a find_in_hash() 
or
find_in_hash_update() lookup. The hash_gc() can then invalidate a pointer which 
the
find_in_hash() or find_in_hash_update() call is still using, which causes a 
segfault.

By simply delaying the hash_gc() to the next find_in_hash() or 
find_in_hash_update()
call, it is no longer possible to have invalid pointers. The suggested patch 
does this
by setting the new 'delayed_hash_gc' flag.


ps. Afaics, it is now also safe to remove all qtime_csenter()/qtime_csexit() 
calls
    in hash.c, but I'll leave that to the author of vde2 to verify that.


Regards,

Bas van Sisseren


-- System Information:
Debian Release: jessie/sid
  APT prefers squeeze-lts
  APT policy: (500, 'squeeze-lts'), (500, 'unstable'), (500, 'testing'), (500, 
'stable'), (500, 'oldstable')
Architecture: i386 (x86_64)
Foreign Architectures: amd64

Kernel: Linux 3.16-2-amd64 (SMP w/4 CPU cores)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)
Shell: /bin/sh linked to /bin/bash

Versions of packages vde2 depends on:
ii  adduser      3.113+nmu3
ii  libc6        2.19-11
ii  libpcap0.8   1.6.2-1
ii  libvde0      2.3.2-4.2
ii  libvdeplug2  2.3.2-4.2

vde2 recommends no packages.

Versions of packages vde2 suggests:
ii  qemu           2.1+dfsg-5
pn  qemu-kvm       <none>
pn  vde2-cryptcab  <none>

-- no debconf information

diff -ru vde2-2.3.2.orig/src/vde_switch/hash.c vde2-2.3.2/src/vde_switch/hash.c
--- vde2-2.3.2.orig/src/vde_switch/hash.c	2011-11-23 17:41:17.000000000 +0100
+++ vde2-2.3.2/src/vde_switch/hash.c	2014-10-07 13:51:35.396596610 +0200
@@ -49,8 +49,11 @@
 	u_int64_t dst;
 };
 
+static int delayed_hash_gc;
 static struct hash_entry **h;
 
+static void hash_gc(void *arg); // forward declaration
+
 static int calc_hash(u_int64_t src)
 {
 	register int x = src * 0x030507090b0d1113LL;
@@ -89,6 +92,7 @@
  * port */
 int find_in_hash(unsigned char *dst,int vlan)
 {
+	if (delayed_hash_gc) hash_gc(NULL);
 	struct hash_entry *e = find_entry(extmac(dst,vlan));
 	if(e == NULL) return -1;
 	return(e->port);
@@ -102,6 +106,7 @@
 	int k = calc_hash(esrc);
 	int oldport;
 	time_t now;
+	if (delayed_hash_gc) hash_gc(NULL);
 	for(e = h[k]; e && e->dst != esrc; e = e->next)
 		;
 	if(e == NULL) {
@@ -267,9 +272,17 @@
 static void hash_gc(void *arg)
 {
 	time_t t = qtime();
+	delayed_hash_gc = 0;
 	for_all_hash(&gc, &t);
 }
 
+/* actual handler which is called every GC_INTERVAL seconds. only set a flag
+   to stay "thread-safe" */
+static void hash_gc_flag(void *arg)
+{
+	delayed_hash_gc++;
+}
+
 #define HASH_INIT(BIT) \
 	({ hash_bits=(BIT);\
 	 hash_mask=HASH_SIZE-1;\
@@ -310,7 +323,7 @@
 {
 	qtimer_del(gc_timerno);
 	gc_interval=p;
-	gc_timerno=qtimer_add(gc_interval,0,hash_gc,NULL);
+	gc_timerno=qtimer_add(gc_interval,0,hash_gc_flag,NULL);
 	return 0;
 }
 
@@ -410,7 +423,7 @@
 
 	gc_interval=GC_INTERVAL;
 	gc_expire=GC_EXPIRE;
-	gc_timerno=qtimer_add(gc_interval,0,hash_gc,NULL);
+	gc_timerno=qtimer_add(gc_interval,0,hash_gc_flag,NULL);
 	ADDCL(cl);
 #ifdef DEBUGOPT
 	ADDDBGCL(dl);