[NETFILTER 00/05]: Netfilter fixes

[NETFILTER 00/05]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these patches for 2.6.25 fix a couple of netfilter bugs: the
smp_processor_id() warning when using preemptible RCU reported
by multiple people, address and state matching in the new
xt_conntrack revision, and improper use of parenthesis in
the NF_QUEUE_NR macro.

Additionally there is a patch to make the NAT core behave similar
to the recently removed SAME target for SNAT, which fixes problems
when accesing certain multihomed sites.

Please apply, thanks.


 include/linux/netfilter.h         |    2 +-
 net/ipv4/netfilter/nf_nat_core.c  |   11 +++++++----
 net/netfilter/nf_conntrack_core.c |   15 ++++++++++++---
 net/netfilter/xt_conntrack.c      |    4 ++--
 4 files changed, 22 insertions(+), 10 deletions(-)

Jan Engelhardt (2):
      [NETFILTER]: xt_conntrack: fix missing boolean clamping
      [NETFILTER]: xt_conntrack: fix IPv4 address comparison

Patrick McHardy (3):
      [NETFILTER]: nf_conntrack: fix smp_processor_id() in preemptible code warning
      [NETFILTER]: nf_nat: always select same SNAT source for same host
      [NETFILTER]: Fix NF_QUEUE_NR() parenthesis

[NETFILTER 01/05]: nf_conntrack: fix smp_processor_id() in preemptible code warning

[Patrick McHardy]

[NETFILTER]: nf_conntrack: fix smp_processor_id() in preemptible code warning

Since we're using RCU for the conntrack hash now, we need to avoid
getting preempted or interrupted by BHs while changing the stats.

Fixes warning reported by Tilman Schmidt <tilman@imap.cc> when using
preemptible RCU:

[   48.180297] BUG: using smp_processor_id() in preemptible [00000000] code: ntpdate/3562
[   48.180297] caller is __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
[   48.180297] Pid: 3562, comm: ntpdate Not tainted 2.6.25-rc2-mm1-testing #1
[   48.180297]  [<c02015b9>] debug_smp_processor_id+0x99/0xb0
[   48.180297]  [<fac643a7>] __nf_conntrack_find+0x9b/0xeb [nf_conntrack]

Tested-by: Tilman Schmidt <tilman@imap.cc>
Tested-by: Christian Casteyde <casteyde.christian@free.fr> [Bugzilla #10097]

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 2ffa8857857148a5b0c0823852d6cb2551a3ae5a
tree 62ff238b8f59b581332c36f703eb4a8878377fae
parent fccf186fa8fba308f4a478691e86399336488dd1
author Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:55 +0100
committer Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:55 +0100

 net/netfilter/nf_conntrack_core.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 327e847..b77eb56 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -256,13 +256,19 @@ __nf_conntrack_find(const struct nf_conntrack_tuple *tuple)
 	struct hlist_node *n;
 	unsigned int hash = hash_conntrack(tuple);
 
+	/* Disable BHs the entire time since we normally need to disable them
+	 * at least once for the stats anyway.
+	 */
+	local_bh_disable();
 	hlist_for_each_entry_rcu(h, n, &nf_conntrack_hash[hash], hnode) {
 		if (nf_ct_tuple_equal(tuple, &h->tuple)) {
 			NF_CT_STAT_INC(found);
+			local_bh_enable();
 			return h;
 		}
 		NF_CT_STAT_INC(searched);
 	}
+	local_bh_enable();
 
 	return NULL;
 }
@@ -400,17 +406,20 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple,
 	struct hlist_node *n;
 	unsigned int hash = hash_conntrack(tuple);
 
-	rcu_read_lock();
+	/* Disable BHs the entire time since we need to disable them at
+	 * least once for the stats anyway.
+	 */
+	rcu_read_lock_bh();
 	hlist_for_each_entry_rcu(h, n, &nf_conntrack_hash[hash], hnode) {
 		if (nf_ct_tuplehash_to_ctrack(h) != ignored_conntrack &&
 		    nf_ct_tuple_equal(tuple, &h->tuple)) {
 			NF_CT_STAT_INC(found);
-			rcu_read_unlock();
+			rcu_read_unlock_bh();
 			return 1;
 		}
 		NF_CT_STAT_INC(searched);
 	}
-	rcu_read_unlock();
+	rcu_read_unlock_bh();
 
 	return 0;
 }

Re: [NETFILTER 01/05]: nf_conntrack: fix smp_processor_id() in preemptible code warning

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 27 Feb 2008 14:14:20 +0100 (MET)

> [NETFILTER]: nf_conntrack: fix smp_processor_id() in preemptible code warning
> 
> Since we're using RCU for the conntrack hash now, we need to avoid
> getting preempted or interrupted by BHs while changing the stats.
> 
> Fixes warning reported by Tilman Schmidt <tilman@imap.cc> when using
> preemptible RCU:
> 
> [   48.180297] BUG: using smp_processor_id() in preemptible [00000000] code: ntpdate/3562
> [   48.180297] caller is __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
> [   48.180297] Pid: 3562, comm: ntpdate Not tainted 2.6.25-rc2-mm1-testing #1
> [   48.180297]  [<c02015b9>] debug_smp_processor_id+0x99/0xb0
> [   48.180297]  [<fac643a7>] __nf_conntrack_find+0x9b/0xeb [nf_conntrack]
> 
> Tested-by: Tilman Schmidt <tilman@imap.cc>
> Tested-by: Christian Casteyde <casteyde.christian@free.fr> [Bugzilla #10097]
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 02/05]: xt_conntrack: fix missing boolean clamping

[Patrick McHardy]

[NETFILTER]: xt_conntrack: fix missing boolean clamping

Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit bcc67d744bd9df15cb4e2d590b30ddadebd5a867
tree 8888741a4eb78658db89b968d066837478238597
parent 2ffa8857857148a5b0c0823852d6cb2551a3ae5a
author Jan Engelhardt <jengelh@computergmbh.de> Wed, 27 Feb 2008 13:23:56 +0100
committer Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:56 +0100

 net/netfilter/xt_conntrack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_conntrack.c b/net/netfilter/xt_conntrack.c
index 8533085..dd192ac 100644
--- a/net/netfilter/xt_conntrack.c
+++ b/net/netfilter/xt_conntrack.c
@@ -231,7 +231,7 @@ conntrack_mt(const struct sk_buff *skb, const struct net_device *in,
 			if (test_bit(IPS_DST_NAT_BIT, &ct->status))
 				statebit |= XT_CONNTRACK_STATE_DNAT;
 		}
-		if ((info->state_mask & statebit) ^
+		if (!!(info->state_mask & statebit) ^
 		    !(info->invert_flags & XT_CONNTRACK_STATE))
 			return false;
 	}

Re: [NETFILTER 02/05]: xt_conntrack: fix missing boolean clamping

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 27 Feb 2008 14:14:22 +0100 (MET)

> [NETFILTER]: xt_conntrack: fix missing boolean clamping
> 
> Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks.

[NETFILTER 03/05]: xt_conntrack: fix IPv4 address comparison

[Patrick McHardy]

[NETFILTER]: xt_conntrack: fix IPv4 address comparison

Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 54b4910d81a787d932d0a3237a8a15879f8eb8b8
tree c1551c39ec76a38a5ee9cb92af07e98ade545bbb
parent bcc67d744bd9df15cb4e2d590b30ddadebd5a867
author Jan Engelhardt <jengelh@computergmbh.de> Wed, 27 Feb 2008 13:23:57 +0100
committer Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:57 +0100

 net/netfilter/xt_conntrack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_conntrack.c b/net/netfilter/xt_conntrack.c
index dd192ac..0c50b28 100644
--- a/net/netfilter/xt_conntrack.c
+++ b/net/netfilter/xt_conntrack.c
@@ -122,7 +122,7 @@ conntrack_addrcmp(const union nf_inet_addr *kaddr,
                   const union nf_inet_addr *umask, unsigned int l3proto)
 {
 	if (l3proto == AF_INET)
-		return (kaddr->ip & umask->ip) == uaddr->ip;
+		return ((kaddr->ip ^ uaddr->ip) & umask->ip) == 0;
 	else if (l3proto == AF_INET6)
 		return ipv6_masked_addr_cmp(&kaddr->in6, &umask->in6,
 		       &uaddr->in6) == 0;

Re: [NETFILTER 03/05]: xt_conntrack: fix IPv4 address comparison

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 27 Feb 2008 14:14:23 +0100 (MET)

> [NETFILTER]: xt_conntrack: fix IPv4 address comparison
> 
> Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 04/05]: nf_nat: always select same SNAT source for same host

[Patrick McHardy]

[NETFILTER]: nf_nat: always select same SNAT source for same host

We've removed the SAME target in 2.6.25-rc since it had 32/64 bit compat
problems and the NAT core provides the same behaviour regarding IP
selection. This turned out to be not entirely correct though, the
NAT core only selects the same IP from a range for the same src,dst
combination. Some people need the same IP for all destinations however.

The easiest way to do this is to ignore the destination IP when
doing SNAT. Since we're using jhash, we still get good distribution
for multiple source IPs.

Tested-by: David Lau <mintypickle@gmail.com>

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit c31b70def66807822dc973e66757507a5fd4c3b6
tree c0a635a5e75bbce4e234ca53d3f12e9af963e741
parent 54b4910d81a787d932d0a3237a8a15879f8eb8b8
author Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:57 +0100
committer Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:57 +0100

 net/ipv4/netfilter/nf_nat_core.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index 0d5fa3a..8e1cae2 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -188,15 +188,19 @@ find_best_ips_proto(struct nf_conntrack_tuple *tuple,
 	__be32 *var_ipp;
 	/* Host order */
 	u_int32_t minip, maxip, j;
+	__be32 dst;
 
 	/* No IP mapping?  Do nothing. */
 	if (!(range->flags & IP_NAT_RANGE_MAP_IPS))
 		return;
 
-	if (maniptype == IP_NAT_MANIP_SRC)
+	if (maniptype == IP_NAT_MANIP_SRC) {
 		var_ipp = &tuple->src.u3.ip;
-	else
+		dst = 0;
+	} else {
 		var_ipp = &tuple->dst.u3.ip;
+		dst = tuple->dst.u3.ip;
+	}
 
 	/* Fast path: only one choice. */
 	if (range->min_ip == range->max_ip) {
@@ -212,8 +216,7 @@ find_best_ips_proto(struct nf_conntrack_tuple *tuple,
 	 * like this), even across reboots. */
 	minip = ntohl(range->min_ip);
 	maxip = ntohl(range->max_ip);
-	j = jhash_2words((__force u32)tuple->src.u3.ip,
-			 (__force u32)tuple->dst.u3.ip, 0);
+	j = jhash_2words((__force u32)tuple->src.u3.ip, (__force u32)dst, 0);
 	j = ((u64)j * (maxip - minip + 1)) >> 32;
 	*var_ipp = htonl(minip + j);
 }

Re: [NETFILTER 04/05]: nf_nat: always select same SNAT source for same host

[Patrick McHardy]

Patrick McHardy wrote:
> [NETFILTER]: nf_nat: always select same SNAT source for same host
> 
> We've removed the SAME target in 2.6.25-rc since it had 32/64 bit compat
> problems and the NAT core provides the same behaviour regarding IP
> selection. This turned out to be not entirely correct though, the
> NAT core only selects the same IP from a range for the same src,dst
> combination. Some people need the same IP for all destinations however.
> 
> The easiest way to do this is to ignore the destination IP when
> doing SNAT. Since we're using jhash, we still get good distribution
> for multiple source IPs.
> 
> Tested-by: David Lau <mintypickle@gmail.com>
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>


Please drop this patch for now, David reported some bad distribution
during further tests that I want to look into.

Re: [NETFILTER 04/05]: nf_nat: always select same SNAT source for same host

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 27 Feb 2008 17:31:59 +0100

> Please drop this patch for now, David reported some bad distribution
> during further tests that I want to look into.

Ok.

Re: [NETFILTER 04/05]: nf_nat: always select same SNAT source for same host

[Bernhard Schmidt]

Patrick McHardy <kaber@trash.net> wrote:

Hello Patrick,

> Patrick McHardy wrote:
>> [NETFILTER]: nf_nat: always select same SNAT source for same host
>> 
>> We've removed the SAME target in 2.6.25-rc since it had 32/64 bit compat
>> problems and the NAT core provides the same behaviour regarding IP
>> selection. This turned out to be not entirely correct though, the
>> NAT core only selects the same IP from a range for the same src,dst
>> combination. Some people need the same IP for all destinations however.
>> 
>> The easiest way to do this is to ignore the destination IP when
>> doing SNAT. Since we're using jhash, we still get good distribution
>> for multiple source IPs.
>> 
>> Tested-by: David Lau <mintypickle@gmail.com>
>> 
>> Signed-off-by: Patrick McHardy <kaber@trash.net>
>
>
> Please drop this patch for now, David reported some bad distribution
> during further tests that I want to look into.

Any news on that? We're getting hit by that issue (ICQ fails to login,
amongst others). In 2.6.25, but I did not see any patch in recent
kernels that changes this.

Bernhard

[NETFILTER 05/05]: Fix NF_QUEUE_NR() parenthesis

[Patrick McHardy]

[NETFILTER]: Fix NF_QUEUE_NR() parenthesis

Properly add parens around the macro argument. This is not needed by
the kernel but the macro is exported to userspace, so it shouldn't
make any assumptions.

Also use NF_VERDICT_BITS instead of NF_VERDICT_QBTIS for the left-shift
since thats whats logically correct.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 81d9aa76da78b14124638e761a4c0acab3d43508
tree d9ec5a1792894c1068aaf4eaab865a5b6001c939
parent c31b70def66807822dc973e66757507a5fd4c3b6
author Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:58 +0100
committer Patrick McHardy <kaber@trash.net> Wed, 27 Feb 2008 13:23:58 +0100

 include/linux/netfilter.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index b74b615..f0680c2 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -31,7 +31,7 @@
 #define NF_VERDICT_QMASK 0xffff0000
 #define NF_VERDICT_QBITS 16
 
-#define NF_QUEUE_NR(x) (((x << NF_VERDICT_QBITS) & NF_VERDICT_QMASK) | NF_QUEUE)
+#define NF_QUEUE_NR(x) ((((x) << NF_VERDICT_BITS) & NF_VERDICT_QMASK) | NF_QUEUE)
 
 /* only for userspace compatibility */
 #ifndef __KERNEL__

Re: [NETFILTER 05/05]: Fix NF_QUEUE_NR() parenthesis

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 27 Feb 2008 14:14:26 +0100 (MET)

> [NETFILTER]: Fix NF_QUEUE_NR() parenthesis
> 
> Properly add parens around the macro argument. This is not needed by
> the kernel but the macro is exported to userspace, so it shouldn't
> make any assumptions.
> 
> Also use NF_VERDICT_BITS instead of NF_VERDICT_QBTIS for the left-shift
> since thats whats logically correct.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks.