[PATCH 0/4] fix SEGV, some run.c API stuff

[PATCH 0/4] fix SEGV, some run.c API stuff

[Max Kellermann]

Hi Pablo,

I found a segmentation fault in conntrackd, here is a possible patch.
Please verify that this is the "real" solution.  This patchset also
includes some patches which change the prototype of __run() to the one
I proposed last week.

Max

---

Max Kellermann (4):
      call __run() after do_alarm_run()
      __run() returns void
      don't ignore fd events when an alarm is due
      check nfct_get_attr()!=NULL


 include/alarm.h |    6 ++++--
 src/alarm.c     |   12 +++++++-----
 src/cache.c     |   46 ++++++++++++++++++++++++++++++++--------------
 src/run.c       |   45 +++++++++++++--------------------------------
 4 files changed, 56 insertions(+), 53 deletions(-)

[PATCH 1/4] check nfct_get_attr()!=NULL

[Max Kellermann]

from time to time, conntrackd segfaults in cache.c, function hash(),
because it does not check the return value of nfct_get_attr().  For
some reason (which I have not investigated), there is no
ATTR_ORIG_IPV4_SRC.  A gdb dump of *ct after the segfault says the
following:

(gdb) p *ct
$3 = {tuple = {{src = {v4 = 4161143072, v6 = {in6_u = {u6_addr8 = " \001\006ø\023\205\000\001\000\000\000\000\000\000\000\002",
            u6_addr16 = {288, 63494, 34067, 256, 0, 0, 0, 512}, u6_addr32 = {4161143072, 16811283, 0, 33554432}}}}, dst = {
        v4 = 4161143072, v6 = {in6_u = {u6_addr8 = " \001\006ø\023\205ÿÿÿÿÿÿÿÿ\000\002", u6_addr16 = {288, 63494, 34067, 65535,
              65535, 65535, 65535, 512}, u6_addr32 = {4161143072, 4294935827, 4294967295, 33619967}}}}, l3protonum = 10 '\n',
      protonum = 6 '\006', l4src = {all = 64727, tcp = {port = 64727}, udp = {port = 64727}, icmp = {type = 215 '×',
          code = 252 'ü', id = 0}, sctp = {port = 64727}}, l4dst = {all = 53767, tcp = {port = 53767}, udp = {port = 53767},
        icmp = {type = 7 '\a', code = 210 'Ò', id = 0}, sctp = {port = 53767}}}, {src = {v4 = 4161143072, v6 = {in6_u = {
            u6_addr8 = " \001\006ø\023\205ÿÿÿÿÿÿÿÿ\000\002", u6_addr16 = {288, 63494, 34067, 65535, 65535, 65535, 65535, 512},
            u6_addr32 = {4161143072, 4294935827, 4294967295, 33619967}}}}, dst = {v4 = 4161143072, v6 = {in6_u = {
            u6_addr8 = " \001\006ø\023\205\000\001\000\000\000\000\000\000\000\002", u6_addr16 = {288, 63494, 34067, 256, 0, 0,
              0, 512}, u6_addr32 = {4161143072, 16811283, 0, 33554432}}}}, l3protonum = 10 '\n', protonum = 6 '\006', l4src = {
        all = 53767, tcp = {port = 53767}, udp = {port = 53767}, icmp = {type = 7 '\a', code = 210 'Ò', id = 0}, sctp = {
          port = 53767}}, l4dst = {all = 64727, tcp = {port = 64727}, udp = {port = 64727}, icmp = {type = 215 '×',
          code = 252 'ü', id = 0}, sctp = {port = 64727}}}, {src = {v4 = 0, v6 = {in6_u = {u6_addr8 = '\0' <repeats 15 times>,
            u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, u6_addr32 = {0, 0, 0, 0}}}}, dst = {v4 = 0, v6 = {in6_u = {
            u6_addr8 = '\0' <repeats 15 times>, u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, u6_addr32 = {0, 0, 0, 0}}}},
      l3protonum = 0 '\0', protonum = 0 '\0', l4src = {all = 0, tcp = {port = 0}, udp = {port = 0}, icmp = {type = 0 '\0',
          code = 0 '\0', id = 0}, sctp = {port = 0}}, l4dst = {all = 0, tcp = {port = 0}, udp = {port = 0}, icmp = {
          type = 0 '\0', code = 0 '\0', id = 0}, sctp = {port = 0}}}}, timeout = 120, mark = 0, secmark = 0, status = 8, use = 0,
  id = 0, protoinfo = {tcp = {state = 1 '\001', flags = {{value = 3 '\003', mask = 0 '\0'}, {value = 0 '\0', mask = 0 '\0'}, {
          value = 0 '\0', mask = 0 '\0'}}}}, counters = {{packets = 0, bytes = 0}, {packets = 0, bytes = 0}, {packets = 0,
      bytes = 0}}, snat = {min_ip = 0, max_ip = 0, l4min = {all = 0, tcp = {port = 0}, udp = {port = 0}, icmp = {type = 0 '\0',
        code = 0 '\0', id = 0}, sctp = {port = 0}}, l4max = {all = 0, tcp = {port = 0}, udp = {port = 0}, icmp = {type = 0 '\0',
        code = 0 '\0', id = 0}, sctp = {port = 0}}}, dnat = {min_ip = 0, max_ip = 0, l4min = {all = 0, tcp = {port = 0}, udp = {
        port = 0}, icmp = {type = 0 '\0', code = 0 '\0', id = 0}, sctp = {port = 0}}, l4max = {all = 0, tcp = {port = 0}, udp = {
        port = 0}, icmp = {type = 0 '\0', code = 0 '\0', id = 0}, sctp = {port = 0}}}, set = {1019888, 31}}
---

 src/cache.c |   46 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 32 insertions(+), 14 deletions(-)


diff --git a/src/cache.c b/src/cache.c
index 2f0e57a..d0fb210 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -29,17 +29,26 @@
 
 static uint32_t hash(const void *data, struct hashtable *table)
 {
+	const void *p;
 	unsigned int a, b;
 	const struct us_conntrack *u = data;
 	struct nf_conntrack *ct = u->ct;
 
-	a = jhash(nfct_get_attr(ct, ATTR_ORIG_IPV4_SRC), sizeof(uint32_t),
-		  ((nfct_get_attr_u8(ct, ATTR_ORIG_L3PROTO) << 16) |
-		   (nfct_get_attr_u8(ct, ATTR_ORIG_L4PROTO))));
-
-	b = jhash(nfct_get_attr(ct, ATTR_ORIG_IPV4_DST), sizeof(uint32_t),
-		  ((nfct_get_attr_u16(ct, ATTR_ORIG_PORT_SRC) << 16) |
-		   (nfct_get_attr_u16(ct, ATTR_ORIG_PORT_DST))));
+	p = nfct_get_attr(ct, ATTR_ORIG_IPV4_SRC);
+	if (p == NULL)
+		a = 0;
+	else
+		a = jhash(p, sizeof(uint32_t),
+			  ((nfct_get_attr_u8(ct, ATTR_ORIG_L3PROTO) << 16) |
+			   (nfct_get_attr_u8(ct, ATTR_ORIG_L4PROTO))));
+
+	p = nfct_get_attr(ct, ATTR_ORIG_IPV4_DST);
+	if (p == NULL)
+		b = 0;
+	else
+		b = jhash(p, sizeof(uint32_t),
+			  ((nfct_get_attr_u16(ct, ATTR_ORIG_PORT_SRC) << 16) |
+			   (nfct_get_attr_u16(ct, ATTR_ORIG_PORT_DST))));
 
 	/*
 	 * Instead of returning hash % table->hashsize (implying a divide)
@@ -53,17 +62,26 @@ static uint32_t hash(const void *data, struct hashtable *table)
 
 static uint32_t hash6(const void *data, struct hashtable *table)
 {
+	const void *p;
 	unsigned int a, b;
 	const struct us_conntrack *u = data;
 	struct nf_conntrack *ct = u->ct;
 
-	a = jhash(nfct_get_attr(ct, ATTR_ORIG_IPV6_SRC), sizeof(uint32_t)*4,
-		  ((nfct_get_attr_u8(ct, ATTR_ORIG_L3PROTO) << 16) |
-		   (nfct_get_attr_u8(ct, ATTR_ORIG_L4PROTO))));
-
-	b = jhash(nfct_get_attr(ct, ATTR_ORIG_IPV6_DST), sizeof(uint32_t)*4,
-		  ((nfct_get_attr_u16(ct, ATTR_ORIG_PORT_SRC) << 16) |
-		   (nfct_get_attr_u16(ct, ATTR_ORIG_PORT_DST))));
+	p = nfct_get_attr(ct, ATTR_ORIG_IPV6_SRC);
+	if (p == NULL)
+		a = 0;
+	else
+		a = jhash(p, sizeof(uint32_t)*4,
+			  ((nfct_get_attr_u8(ct, ATTR_ORIG_L3PROTO) << 16) |
+			   (nfct_get_attr_u8(ct, ATTR_ORIG_L4PROTO))));
+
+	p = nfct_get_attr(ct, ATTR_ORIG_IPV6_DST);
+	if (p == NULL)
+		b = 0;
+	else
+		b = jhash(nfct_get_attr(ct, ATTR_ORIG_IPV6_DST), sizeof(uint32_t)*4,
+			  ((nfct_get_attr_u16(ct, ATTR_ORIG_PORT_SRC) << 16) |
+			   (nfct_get_attr_u16(ct, ATTR_ORIG_PORT_DST))));
 
 	return ((uint64_t)jhash_2words(a, b, 0) * table->hashsize) >> 32;
 }


-
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] check nfct_get_attr()!=NULL

[Pablo Neira Ayuso]

Max Kellermann wrote:
> from time to time, conntrackd segfaults in cache.c, function hash(),
> because it does not check the return value of nfct_get_attr().  For
> some reason (which I have not investigated), there is no
> ATTR_ORIG_IPV4_SRC.  A gdb dump of *ct after the segfault says the
> following:

I cannot reproduce this problem in my testbed. Are you running
conntrackd with IPv6 connections?

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH 1/4] check nfct_get_attr()!=NULL

[Max Kellermann]

On 2008/01/21 13:53, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Max Kellermann wrote:
> > from time to time, conntrackd segfaults in cache.c, function hash(),
> > because it does not check the return value of nfct_get_attr().  For
> > some reason (which I have not investigated), there is no
> > ATTR_ORIG_IPV4_SRC.  A gdb dump of *ct after the segfault says the
> > following:
> 
> I cannot reproduce this problem in my testbed. Are you running
> conntrackd with IPv6 connections?

This machine uses IPv6 and IPv6 CT.  I have no idea what conntrackd
does with these.

Max

Re: [PATCH 1/4] check nfct_get_attr()!=NULL

[Pablo Neira Ayuso]

Max Kellermann wrote:
> On 2008/01/21 13:53, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> Max Kellermann wrote:
>>> from time to time, conntrackd segfaults in cache.c, function hash(),
>>> because it does not check the return value of nfct_get_attr().  For
>>> some reason (which I have not investigated), there is no
>>> ATTR_ORIG_IPV4_SRC.  A gdb dump of *ct after the segfault says the
>>> following:
>> I cannot reproduce this problem in my testbed. Are you running
>> conntrackd with IPv6 connections?
> 
> This machine uses IPv6 and IPv6 CT.  I have no idea what conntrackd
> does with these.

I started IPv6 support months ago but it's still incomplete. This is
probably the reason of this crash. We can fix the IPv6 code or
preventively disable it until we finish it.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[PATCH 2/4] don't ignore fd events when an alarm is due

[Max Kellermann]

also, pass &next_alarm to __run() only if there is an alarm; eliminate
the "timeout" parameter; the alarm functions get_next_alarm_run() and
do_alarm_run() return an timeval pointer instead of a boolean.
---

 include/alarm.h |    6 ++++--
 src/alarm.c     |   12 +++++++-----
 src/run.c       |   38 ++++++++++++--------------------------
 3 files changed, 23 insertions(+), 33 deletions(-)


diff --git a/include/alarm.h b/include/alarm.h
index f0bbb95..3569025 100644
--- a/include/alarm.h
+++ b/include/alarm.h
@@ -26,8 +26,10 @@ void add_alarm(struct alarm_list *alarm, unsigned long sc, unsigned long usc);
 
 void del_alarm(struct alarm_list *alarm);
 
-int get_next_alarm_run(struct timeval *next_alarm);
+struct timeval *
+get_next_alarm_run(struct timeval *next_alarm);
 
-int do_alarm_run(struct timeval *next_alarm);
+struct timeval *
+do_alarm_run(struct timeval *next_alarm);
 
 #endif
diff --git a/src/alarm.c b/src/alarm.c
index 2190bb6..883a42c 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -74,7 +74,7 @@ void del_alarm(struct alarm_list *alarm)
 	}
 }
 
-static int 
+static struct timeval *
 calculate_next_run(struct timeval *cand,
 		   struct timeval *tv, 
 		   struct timeval *next_run)
@@ -87,12 +87,13 @@ calculate_next_run(struct timeval *cand,
 			next_run->tv_sec = 0;
 			next_run->tv_usec = 0;
 		}
-		return 1;
+		return next_run;
 	}
-	return 0;
+	return NULL;
 }
 
-int get_next_alarm_run(struct timeval *next_run)
+struct timeval *
+get_next_alarm_run(struct timeval *next_run)
 {
 	int i;
 	struct alarm_list *t;
@@ -133,7 +134,8 @@ tv_compare(struct alarm_list *a, struct timeval *cur, struct timeval *cand)
 	return 0;
 }
 
-int do_alarm_run(struct timeval *next_run)
+struct timeval *
+do_alarm_run(struct timeval *next_run)
 {
 	int i;
 	struct alarm_list *t, *next, *prev;
diff --git a/src/run.c b/src/run.c
index efeda5a..2d51ca1 100644
--- a/src/run.c
+++ b/src/run.c
@@ -158,15 +158,10 @@ init(void)
 	return 0;
 }
 
-static int __run(struct timeval *next_alarm, int *timeout)
+static int __run(struct timeval *next_alarm)
 {
 	int max, ret;
 	fd_set readfds;
-	struct timeval *tmp = next_alarm;
-
-	/* No alarms, select must block */
-	if (*timeout == 0)
-		tmp = NULL;
 
 	FD_ZERO(&readfds);
 	FD_SET(STATE(local), &readfds);
@@ -177,7 +172,7 @@ static int __run(struct timeval *next_alarm, int *timeout)
 	if (STATE(mode)->add_fds_to_set)
 		max = MAX(max, STATE(mode)->add_fds_to_set(&readfds));
 
-	ret = select(max+1, &readfds, NULL, NULL, tmp);
+	ret = select(max+1, &readfds, NULL, NULL, next_alarm);
 	if (ret == -1) {
 		/* interrupted syscall, retry */
 		if (errno == EINTR)
@@ -188,10 +183,6 @@ static int __run(struct timeval *next_alarm, int *timeout)
 		return 0;
 	}
 
-	/* timeout expired, run the alarm list */
-	if (tmp != NULL && !timerisset(tmp))
-		return 1;
-
 	/* signals are racy */
 	sigprocmask(SIG_BLOCK, &STATE(block), NULL);		
 
@@ -235,14 +226,6 @@ static int __run(struct timeval *next_alarm, int *timeout)
 	if (STATE(mode)->run)
 		STATE(mode)->run(&readfds);
 
-	/* check if we have introduced any new alarms */
-	if (*timeout == 0 && alarm_counter > 0) {
-		*timeout = 1;
-		if (!get_next_alarm_run(next_alarm))
-			dlog(LOG_ERR, "Bug in alarm?");
-		return 0;
-	}
-
 	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
 
 	return 0;
@@ -251,17 +234,20 @@ static int __run(struct timeval *next_alarm, int *timeout)
 void __attribute__((noreturn))
 run(void)
 {
-	int timeout;
 	struct timeval next_alarm; 
+	struct timeval *next;
 
 	/* initialization: get the next alarm available */
-	timeout = get_next_alarm_run(&next_alarm);
+	next = get_next_alarm_run(&next_alarm);
 
 	while(1) {
-		if (__run(&next_alarm, &timeout)) {
-			sigprocmask(SIG_BLOCK, &STATE(block), NULL);
-			timeout = do_alarm_run(&next_alarm);
-			sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
-		}
+		__run(next);
+
+		sigprocmask(SIG_BLOCK, &STATE(block), NULL);
+		if (next != NULL && timerisset(next))
+			next = do_alarm_run(&next_alarm);
+		else
+			next = get_next_alarm_run(&next_alarm);
+		sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
 	}
 }

Re: [PATCH 2/4] don't ignore fd events when an alarm is due

[Pablo Neira Ayuso]

Max Kellermann wrote:
> also, pass &next_alarm to __run() only if there is an alarm; eliminate
> the "timeout" parameter; the alarm functions get_next_alarm_run() and
> do_alarm_run() return an timeval pointer instead of a boolean.

Hm, this patch makes my CPU suck up after the first alarm run.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH 2/4] don't ignore fd events when an alarm is due

[Max Kellermann]

On 2008/01/21 14:14, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Max Kellermann wrote:
> > also, pass &next_alarm to __run() only if there is an alarm; eliminate
> > the "timeout" parameter; the alarm functions get_next_alarm_run() and
> > do_alarm_run() return an timeval pointer instead of a boolean.
> 
> Hm, this patch makes my CPU suck up after the first alarm run.

Damn.  I forgot a "!".

commit 02683a053a26e142e8067b0e884ff5c49af908d4
Author: Max Kellermann <max@duempel.org>
Date:   Mon Jan 21 15:13:38 2008 +0100

    added "!" before timerisset(next)

diff --git a/src/run.c b/src/run.c
index 718e63c..fe57858 100644
--- a/src/run.c
+++ b/src/run.c
@@ -237,7 +237,7 @@ run(void)
 
 	while(1) {
 		sigprocmask(SIG_BLOCK, &STATE(block), NULL);
-		if (next != NULL && timerisset(next))
+		if (next != NULL && !timerisset(next))
 			next = do_alarm_run(&next_alarm);
 		else
 			next = get_next_alarm_run(&next_alarm);

Re: [PATCH 2/4] don't ignore fd events when an alarm is due

[Pablo Neira Ayuso]

Max Kellermann wrote:
> also, pass &next_alarm to __run() only if there is an alarm; eliminate
> the "timeout" parameter; the alarm functions get_next_alarm_run() and
> do_alarm_run() return an timeval pointer instead of a boolean.

Applied with the missing ! in timerisset(). Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[PATCH 3/4] __run() returns void

[Max Kellermann]

---

 src/run.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)


diff --git a/src/run.c b/src/run.c
index 2d51ca1..b24cd9b 100644
--- a/src/run.c
+++ b/src/run.c
@@ -158,7 +158,7 @@ init(void)
 	return 0;
 }
 
-static int __run(struct timeval *next_alarm)
+static void __run(struct timeval *next_alarm)
 {
 	int max, ret;
 	fd_set readfds;
@@ -176,11 +176,11 @@ static int __run(struct timeval *next_alarm)
 	if (ret == -1) {
 		/* interrupted syscall, retry */
 		if (errno == EINTR)
-			return 0;
+			return;
 
 		dlog(LOG_WARNING, 
 		     "select failed: %s", strerror(errno));
-		return 0;
+		return;
 	}
 
 	/* signals are racy */
@@ -227,8 +227,6 @@ static int __run(struct timeval *next_alarm)
 		STATE(mode)->run(&readfds);
 
 	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
-
-	return 0;
 }
 
 void __attribute__((noreturn))

Re: [PATCH 3/4] __run() returns void

[Pablo Neira Ayuso]

Max Kellermann wrote:
>  src/run.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)

I have kept back 3/4 and 4/4 until we find the issue with patch 2/4.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH 3/4] __run() returns void

[Pablo Neira Ayuso]

Max Kellermann wrote:
>  src/run.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)

Applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[PATCH 4/4] call __run() after do_alarm_run()

[Max Kellermann]

this saves us the initialization.  this patch is re-sent, it was first
submitted on january 18th.
---

 src/run.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)


diff --git a/src/run.c b/src/run.c
index b24cd9b..718e63c 100644
--- a/src/run.c
+++ b/src/run.c
@@ -233,19 +233,16 @@ void __attribute__((noreturn))
 run(void)
 {
 	struct timeval next_alarm; 
-	struct timeval *next;
-
-	/* initialization: get the next alarm available */
-	next = get_next_alarm_run(&next_alarm);
+	struct timeval *next = NULL;
 
 	while(1) {
-		__run(next);
-
 		sigprocmask(SIG_BLOCK, &STATE(block), NULL);
 		if (next != NULL && timerisset(next))
 			next = do_alarm_run(&next_alarm);
 		else
 			next = get_next_alarm_run(&next_alarm);
 		sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
+
+		__run(next);
 	}
 }

Re: [PATCH 4/4] call __run() after do_alarm_run()

[Pablo Neira Ayuso]

Max Kellermann wrote:
> this saves us the initialization.  this patch is re-sent, it was first
> submitted on january 18th.

Applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers