[NETFILTER 00/03]: Netfilter fixes

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

following are three netfilter fixes for 2.6.20, fixing a problem with ICMP
translation in the new nf_nat code and two bugs in the new PPTP helper port
breaking NAT of PPTP connections.

Please apply, thanks.


 net/ipv4/netfilter/Makefile       |   20 ++++++++++----------
 net/ipv4/netfilter/nf_nat_pptp.c  |    4 ++--
 net/netfilter/nf_conntrack_pptp.c |    2 +-
 3 files changed, 13 insertions(+), 13 deletions(-)

Patrick McHardy:
      [NETFILTER]: nf_nat: fix ICMP translation with statically linked conntrack
      [NETFILTER]: nf_nat_pptp: fix expectation removal
      [NETFILTER]: nf_conntrack_pptp: fix NAT setup of expected GRE connections

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 25 Jan 2007 01:21:56 +0100 (MET)

> following are three netfilter fixes for 2.6.20, fixing a problem with ICMP
> translation in the new nf_nat code and two bugs in the new PPTP helper port
> breaking NAT of PPTP connections.
> 
> Please apply, thanks.

All applied, thanks a lot Patrick.

Re: [NETFILTER 00/03]: Netfilter fixes

[Jorge Bastos]

David,
I have kernel 2.6.20-rc6 and i can't make pptp connections, only 2.6.20-rc5 
with the patch patrick provided me.
In wich version did you apply this?

Jorge



----- Original Message ----- 
From: "David Miller" <davem@davemloft.net>
To: <kaber@trash.net>
Cc: <netfilter-devel@lists.netfilter.org>
Sent: Friday, January 26, 2007 9:08 AM
Subject: Re: [NETFILTER 00/03]: Netfilter fixes


> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 25 Jan 2007 01:21:56 +0100 (MET)
>
>> following are three netfilter fixes for 2.6.20, fixing a problem with 
>> ICMP
>> translation in the new nf_nat code and two bugs in the new PPTP helper 
>> port
>> breaking NAT of PPTP connections.
>>
>> Please apply, thanks.
>
> All applied, thanks a lot Patrick.
>
> 

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

following are a few more netfilter fixes for 2.6.20, fixing a division
by zero in the connbytes match (I will pass this one on to -stable as
well) and two problems with the SIP conntrack helper.

Please apply, thanks.


 net/ipv4/netfilter/ip_conntrack_sip.c |   10 ++++++++--
 net/netfilter/nf_conntrack_sip.c      |   10 ++++++++--
 net/netfilter/xt_connbytes.c          |   29 ++++++++++++-----------------
 3 files changed, 28 insertions(+), 21 deletions(-)

Lars Immisch:
      [NETFILTER]: SIP conntrack: fix skipping over user info in SIP headers

Patrick McHardy:
      [NETFILTER]: xt_connbytes: fix division by zero
      [NETFILTER]: SIP conntrack: fix out of bounds memory access

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 30 Jan 2007 19:16:27 +0100 (MET)

> Hi Dave,
> 
> following are a few more netfilter fixes for 2.6.20, fixing a division
> by zero in the connbytes match (I will pass this one on to -stable as
> well) and two problems with the SIP conntrack helper.
> 
> Please apply, thanks.

I sucked these all in, please push that one to -stable, thanks.

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

following are three more patches for some nasty netfilter bugs, fixing incorrect
conntrack classification of IPv6 fragments, a crash in nfnetlink_log with briding
and a missing terminating zero-byte in the nfnetlink_log prefix message.

Please apply, thanks.


 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    1 +
 net/netfilter/nfnetlink_log.c                  |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

Patrick McHardy:
      [NETFILTER]: nf_conntrack: fix incorrect classification of IPv6 fragments as ESTABLISHED
      [NETFILTER]: nfnetlink_log: zero-terminate prefix
      [NETFILTER]: nfnetlink_log: fix crash on bridged packet

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue,  6 Mar 2007 08:44:01 +0100 (MET)

> Hi Dave,
> 
> following are three more patches for some nasty netfilter bugs, fixing incorrect
> conntrack classification of IPv6 fragments, a crash in nfnetlink_log with briding
> and a missing terminating zero-byte in the nfnetlink_log prefix message.
> 
> Please apply, thanks.

All 3 patches applied, thank you.

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these patches fix improper textsearch_prepare return value checks in the amanda
conntrack helper, the iptables compat crash reported by Jan Engelhardt and some
connection tracking helper unload races.

Please apply, thanks.


 include/linux/netfilter_ipv4/ip_tables.h       |   17 +++++
 net/ipv4/netfilter/ip_tables.c                 |   81 +++++++++++++++++++------
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   13 ++--
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    9 ++
 net/netfilter/nf_conntrack_amanda.c            |   12 +--
 net/netfilter/nf_conntrack_core.c              |   26 +++++---
 net/netfilter/nf_conntrack_expect.c            |    4 +
 net/netfilter/nf_conntrack_helper.c            |    2 
 net/netfilter/nf_conntrack_netlink.c           |   34 +++++++---
 net/netfilter/nf_conntrack_proto_gre.c         |    2 
 10 files changed, 147 insertions(+), 53 deletions(-)

Akinobu Mita (1):
      [NETFILTER]: nf_conntrack_amanda: fix textsearch_prepare() error check

Dmitry Mishin (1):
      [NETFILTER]: ip_tables: fix compat related crash

Patrick McHardy (1):
      [NETFILTER]: nf_conntrack: fix helper module unload races

[NETFILTER 01/03]: nf_conntrack: fix helper module unload races

[Patrick McHardy]

[NETFILTER]: nf_conntrack: fix helper module unload races

When a helper module is unloaded all conntracks refering to it have their
helper pointer NULLed out, leading to lots of races. In most places this
can be fixed by proper use of RCU (they do already check for != NULL,
but in a racy way), additionally nf_conntrack_expect_related needs to
bail out when no helper is present.

Also remove two paranoid BUG_ONs in nf_conntrack_proto_gre that are racy
and not worth fixing.

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

---
commit eed17841cda83ffa195b9e5ec4d5ee4b6840ed17
tree 1bca5e37fc461999f2160c3b0673a12649ea2023
parent 5ecd3100e695228ac5e0ce0e325e252c0f11806f
author Patrick McHardy <kaber@trash.net> Tue, 05 Jun 2007 14:22:34 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 05 Jun 2007 14:22:34 +0200

 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   13 ++++++---
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    9 +++++-
 net/netfilter/nf_conntrack_core.c              |   26 +++++++++++++-----
 net/netfilter/nf_conntrack_expect.c            |    4 +++
 net/netfilter/nf_conntrack_helper.c            |    2 +
 net/netfilter/nf_conntrack_netlink.c           |   34 ++++++++++++++++--------
 net/netfilter/nf_conntrack_proto_gre.c         |    2 -
 7 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index fd62a41..6dc72a8 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -133,6 +133,7 @@ static unsigned int ipv4_conntrack_help(unsigned int hooknum,
 	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn_help *help;
+	struct nf_conntrack_helper *helper;
 
 	/* This is where we call the helper: as the packet goes out. */
 	ct = nf_ct_get(*pskb, &ctinfo);
@@ -140,12 +141,14 @@ static unsigned int ipv4_conntrack_help(unsigned int hooknum,
 		return NF_ACCEPT;
 
 	help = nfct_help(ct);
-	if (!help || !help->helper)
+	if (!help)
 		return NF_ACCEPT;
-
-	return help->helper->help(pskb,
-				  skb_network_offset(*pskb) + ip_hdrlen(*pskb),
-				  ct, ctinfo);
+	/* rcu_read_lock()ed by nf_hook_slow */
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		return NF_ACCEPT;
+	return helper->help(pskb, skb_network_offset(*pskb) + ip_hdrlen(*pskb),
+			    ct, ctinfo);
 }
 
 static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index dc442fb..1b1797f 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -160,6 +160,7 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
 {
 	struct nf_conn *ct;
 	struct nf_conn_help *help;
+	struct nf_conntrack_helper *helper;
 	enum ip_conntrack_info ctinfo;
 	unsigned int ret, protoff;
 	unsigned int extoff = (u8 *)(ipv6_hdr(*pskb) + 1) - (*pskb)->data;
@@ -172,7 +173,11 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
 		goto out;
 
 	help = nfct_help(ct);
-	if (!help || !help->helper)
+	if (!help)
+		goto out;
+	/* rcu_read_lock()ed by nf_hook_slow */
+	helper = rcu_dereference(help->helper);
+	if (!helper)
 		goto out;
 
 	protoff = nf_ct_ipv6_skip_exthdr(*pskb, extoff, &pnum,
@@ -182,7 +187,7 @@ static unsigned int ipv6_confirm(unsigned int hooknum,
 		return NF_ACCEPT;
 	}
 
-	ret = help->helper->help(pskb, protoff, ct, ctinfo);
+	ret = helper->help(pskb, protoff, ct, ctinfo);
 	if (ret != NF_ACCEPT)
 		return ret;
 out:
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 483e927..7a15e30 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -350,9 +350,15 @@ static void death_by_timeout(unsigned long ul_conntrack)
 {
 	struct nf_conn *ct = (void *)ul_conntrack;
 	struct nf_conn_help *help = nfct_help(ct);
+	struct nf_conntrack_helper *helper;
 
-	if (help && help->helper && help->helper->destroy)
-		help->helper->destroy(ct);
+	if (help) {
+		rcu_read_lock();
+		helper = rcu_dereference(help->helper);
+		if (helper && helper->destroy)
+			helper->destroy(ct);
+		rcu_read_unlock();
+	}
 
 	write_lock_bh(&nf_conntrack_lock);
 	/* Inside lock so preempt is disabled on module removal path.
@@ -661,6 +667,7 @@ init_conntrack(const struct nf_conntrack_tuple *tuple,
 	       unsigned int dataoff)
 {
 	struct nf_conn *conntrack;
+	struct nf_conn_help *help;
 	struct nf_conntrack_tuple repl_tuple;
 	struct nf_conntrack_expect *exp;
 	u_int32_t features = 0;
@@ -691,6 +698,7 @@ init_conntrack(const struct nf_conntrack_tuple *tuple,
 	write_lock_bh(&nf_conntrack_lock);
 	exp = find_expectation(tuple);
 
+	help = nfct_help(conntrack);
 	if (exp) {
 		DEBUGP("conntrack: expectation arrives ct=%p exp=%p\n",
 			conntrack, exp);
@@ -698,7 +706,7 @@ init_conntrack(const struct nf_conntrack_tuple *tuple,
 		__set_bit(IPS_EXPECTED_BIT, &conntrack->status);
 		conntrack->master = exp->master;
 		if (exp->helper)
-			nfct_help(conntrack)->helper = exp->helper;
+			rcu_assign_pointer(help->helper, exp->helper);
 #ifdef CONFIG_NF_CONNTRACK_MARK
 		conntrack->mark = exp->master->mark;
 #endif
@@ -708,10 +716,11 @@ init_conntrack(const struct nf_conntrack_tuple *tuple,
 		nf_conntrack_get(&conntrack->master->ct_general);
 		NF_CT_STAT_INC(expect_new);
 	} else {
-		struct nf_conn_help *help = nfct_help(conntrack);
-
-		if (help)
-			help->helper = __nf_ct_helper_find(&repl_tuple);
+		if (help) {
+			/* not in hash table yet, so not strictly necessary */
+			rcu_assign_pointer(help->helper,
+					   __nf_ct_helper_find(&repl_tuple));
+		}
 		NF_CT_STAT_INC(new);
 	}
 
@@ -893,7 +902,8 @@ void nf_conntrack_alter_reply(struct nf_conn *ct,
 		helper = __nf_ct_helper_find(newreply);
 		if (helper)
 			memset(&help->help, 0, sizeof(help->help));
-		help->helper = helper;
+		/* not in hash table yet, so not strictly necessary */
+		rcu_assign_pointer(help->helper, helper);
 	}
 	write_unlock_bh(&nf_conntrack_lock);
 }
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 117cbfd..504fb6c 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -337,6 +337,10 @@ int nf_conntrack_expect_related(struct nf_conntrack_expect *expect)
 	NF_CT_ASSERT(master_help);
 
 	write_lock_bh(&nf_conntrack_lock);
+	if (!master_help->helper) {
+		ret = -ESHUTDOWN;
+		goto out;
+	}
 	list_for_each_entry(i, &nf_conntrack_expect_list, list) {
 		if (expect_matches(i, expect)) {
 			/* Refresh timer: if it's dying, ignore.. */
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 0743be4..f868b7f 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -93,7 +93,7 @@ static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 
 	if (help && help->helper == me) {
 		nf_conntrack_event(IPCT_HELPER, ct);
-		help->helper = NULL;
+		rcu_assign_pointer(help->helper, NULL);
 	}
 	return 0;
 }
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index d6d39e2..3f73327 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -171,21 +171,29 @@ ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct)
 {
 	struct nfattr *nest_helper;
 	const struct nf_conn_help *help = nfct_help(ct);
+	struct nf_conntrack_helper *helper;
 
-	if (!help || !help->helper)
+	if (!help)
 		return 0;
 
+	rcu_read_lock();
+	helper = rcu_dereference(help->helper);
+	if (!helper)
+		goto out;
+
 	nest_helper = NFA_NEST(skb, CTA_HELP);
-	NFA_PUT(skb, CTA_HELP_NAME, strlen(help->helper->name), help->helper->name);
+	NFA_PUT(skb, CTA_HELP_NAME, strlen(helper->name), helper->name);
 
-	if (help->helper->to_nfattr)
-		help->helper->to_nfattr(skb, ct);
+	if (helper->to_nfattr)
+		helper->to_nfattr(skb, ct);
 
 	NFA_NEST_END(skb, nest_helper);
-
+out:
+	rcu_read_unlock();
 	return 0;
 
 nfattr_failure:
+	rcu_read_unlock();
 	return -1;
 }
 
@@ -842,7 +850,7 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nfattr *cda[])
 		if (help && help->helper) {
 			/* we had a helper before ... */
 			nf_ct_remove_expectations(ct);
-			help->helper = NULL;
+			rcu_assign_pointer(help->helper, NULL);
 		}
 
 		return 0;
@@ -866,7 +874,7 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nfattr *cda[])
 
 	/* need to zero data of old helper */
 	memset(&help->help, 0, sizeof(help->help));
-	help->helper = helper;
+	rcu_assign_pointer(help->helper, helper);
 
 	return 0;
 }
@@ -950,6 +958,7 @@ ctnetlink_create_conntrack(struct nfattr *cda[],
 	struct nf_conn *ct;
 	int err = -EINVAL;
 	struct nf_conn_help *help;
+	struct nf_conntrack_helper *helper = NULL;
 
 	ct = nf_conntrack_alloc(otuple, rtuple);
 	if (ct == NULL || IS_ERR(ct))
@@ -980,14 +989,17 @@ ctnetlink_create_conntrack(struct nfattr *cda[],
 #endif
 
 	help = nfct_help(ct);
-	if (help)
-		help->helper = nf_ct_helper_find_get(rtuple);
+	if (help) {
+		helper = nf_ct_helper_find_get(rtuple);
+		/* not in hash table yet so not strictly necessary */
+		rcu_assign_pointer(help->helper, helper);
+	}
 
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
 
-	if (help && help->helper)
-		nf_ct_helper_put(help->helper);
+	if (helper)
+		nf_ct_helper_put(helper);
 
 	return 0;
 
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 5434472..339c397 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -100,7 +100,6 @@ int nf_ct_gre_keymap_add(struct nf_conn *ct, enum ip_conntrack_dir dir,
 	struct nf_conn_help *help = nfct_help(ct);
 	struct nf_ct_gre_keymap **kmp, *km;
 
-	BUG_ON(strcmp(help->helper->name, "pptp"));
 	kmp = &help->help.ct_pptp_info.keymap[dir];
 	if (*kmp) {
 		/* check whether it's a retransmission */
@@ -137,7 +136,6 @@ void nf_ct_gre_keymap_destroy(struct nf_conn *ct)
 	enum ip_conntrack_dir dir;
 
 	DEBUGP("entering for ct %p\n", ct);
-	BUG_ON(strcmp(help->helper->name, "pptp"));
 
 	write_lock_bh(&nf_ct_gre_lock);
 	for (dir = IP_CT_DIR_ORIGINAL; dir < IP_CT_DIR_MAX; dir++) {

[NETFILTER 02/03]: ip_tables: fix compat related crash

[Patrick McHardy]

[NETFILTER]: ip_tables: fix compat related crash

check_compat_entry_size_and_hooks iterates over the matches and calls
compat_check_calc_match, which loads the match and calculates the
compat offsets, but unlike the non-compat version, doesn't call
->checkentry yet. On error however it calls cleanup_matches, which in
turn calls ->destroy, which can result in crashes if the destroy
function (validly) expects to only get called after the checkentry
function.

Add a compat_release_match function that only drops the module reference
on error and rename compat_check_calc_match to compat_find_calc_match to
reflect the fact that it doesn't call the checkentry function.

Reported by Jan Engelhardt <jengelh@linux01.gwdg.de>

Signed-off-by: Dmitry Mishin <dim@openvz.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit b14c27ef9486854969ae471aa5818b1e1352a0d7
tree 9ad281718d3780c20b7dc147d7ff8be0d0bbf298
parent eed17841cda83ffa195b9e5ec4d5ee4b6840ed17
author Dmitry Mishin <dim@openvz.org> Tue, 05 Jun 2007 15:33:02 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 05 Jun 2007 15:33:02 +0200

 include/linux/netfilter_ipv4/ip_tables.h |   20 +++++++
 net/ipv4/netfilter/ip_tables.c           |   81 +++++++++++++++++++++++-------
 2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/include/linux/netfilter_ipv4/ip_tables.h b/include/linux/netfilter_ipv4/ip_tables.h
index 2f46dd7..e992cd6 100644
--- a/include/linux/netfilter_ipv4/ip_tables.h
+++ b/include/linux/netfilter_ipv4/ip_tables.h
@@ -264,6 +264,26 @@ ipt_get_target(struct ipt_entry *e)
 	__ret;							\
 })
 
+/* fn returns 0 to continue iteration */
+#define IPT_ENTRY_ITERATE_CONTINUE(entries, size, n, fn, args...) \
+({								\
+	unsigned int __i, __n;					\
+	int __ret = 0;						\
+	struct ipt_entry *__entry;				\
+								\
+	for (__i = 0, __n = 0; __i < (size);			\
+	     __i += __entry->next_offset, __n++) { 		\
+		__entry = (void *)(entries) + __i;		\
+		if (__n < n)					\
+			continue;				\
+								\
+		__ret = fn(__entry , ## args);			\
+		if (__ret != 0)					\
+			break;					\
+	}							\
+	__ret;							\
+})
+
 /*
  *	Main firewall chains definitions and global var's definitions.
  */
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e3f83bf..9bacf1a 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -499,7 +499,8 @@ check_entry(struct ipt_entry *e, const char *name)
 }
 
 static inline int check_match(struct ipt_entry_match *m, const char *name,
-				const struct ipt_ip *ip, unsigned int hookmask)
+				const struct ipt_ip *ip, unsigned int hookmask,
+				unsigned int *i)
 {
 	struct xt_match *match;
 	int ret;
@@ -515,6 +516,8 @@ static inline int check_match(struct ipt_entry_match *m, const char *name,
 			 m->u.kernel.match->name);
 		ret = -EINVAL;
 	}
+	if (!ret)
+		(*i)++;
 	return ret;
 }
 
@@ -537,11 +540,10 @@ find_check_match(struct ipt_entry_match *m,
 	}
 	m->u.kernel.match = match;
 
-	ret = check_match(m, name, ip, hookmask);
+	ret = check_match(m, name, ip, hookmask, i);
 	if (ret)
 		goto err;
 
-	(*i)++;
 	return 0;
 err:
 	module_put(m->u.kernel.match->me);
@@ -1425,7 +1427,7 @@ out:
 }
 
 static inline int
-compat_check_calc_match(struct ipt_entry_match *m,
+compat_find_calc_match(struct ipt_entry_match *m,
 	    const char *name,
 	    const struct ipt_ip *ip,
 	    unsigned int hookmask,
@@ -1449,6 +1451,31 @@ compat_check_calc_match(struct ipt_entry_match *m,
 }
 
 static inline int
+compat_release_match(struct ipt_entry_match *m, unsigned int *i)
+{
+	if (i && (*i)-- == 0)
+		return 1;
+
+	module_put(m->u.kernel.match->me);
+	return 0;
+}
+
+static inline int
+compat_release_entry(struct ipt_entry *e, unsigned int *i)
+{
+	struct ipt_entry_target *t;
+
+	if (i && (*i)-- == 0)
+		return 1;
+
+	/* Cleanup all matches */
+	IPT_MATCH_ITERATE(e, compat_release_match, NULL);
+	t = ipt_get_target(e);
+	module_put(t->u.kernel.target->me);
+	return 0;
+}
+
+static inline int
 check_compat_entry_size_and_hooks(struct ipt_entry *e,
 			   struct xt_table_info *newinfo,
 			   unsigned int *size,
@@ -1485,10 +1512,10 @@ check_compat_entry_size_and_hooks(struct ipt_entry *e,
 	off = 0;
 	entry_offset = (void *)e - (void *)base;
 	j = 0;
-	ret = IPT_MATCH_ITERATE(e, compat_check_calc_match, name, &e->ip,
+	ret = IPT_MATCH_ITERATE(e, compat_find_calc_match, name, &e->ip,
 			e->comefrom, &off, &j);
 	if (ret != 0)
-		goto cleanup_matches;
+		goto release_matches;
 
 	t = ipt_get_target(e);
 	target = try_then_request_module(xt_find_target(AF_INET,
@@ -1499,7 +1526,7 @@ check_compat_entry_size_and_hooks(struct ipt_entry *e,
 		duprintf("check_compat_entry_size_and_hooks: `%s' not found\n",
 							t->u.user.name);
 		ret = target ? PTR_ERR(target) : -ENOENT;
-		goto cleanup_matches;
+		goto release_matches;
 	}
 	t->u.kernel.target = target;
 
@@ -1526,8 +1553,8 @@ check_compat_entry_size_and_hooks(struct ipt_entry *e,
 
 out:
 	module_put(t->u.kernel.target->me);
-cleanup_matches:
-	IPT_MATCH_ITERATE(e, cleanup_match, &j);
+release_matches:
+	IPT_MATCH_ITERATE(e, compat_release_match, &j);
 	return ret;
 }
 
@@ -1574,15 +1601,26 @@ static int compat_copy_entry_from_user(struct ipt_entry *e, void **dstptr,
 	return ret;
 }
 
-static inline int compat_check_entry(struct ipt_entry *e, const char *name)
+static inline int compat_check_entry(struct ipt_entry *e, const char *name,
+						unsigned int *i)
 {
-	int ret;
+	int j, ret;
 
-	ret = IPT_MATCH_ITERATE(e, check_match, name, &e->ip, e->comefrom);
+	j = 0;
+	ret = IPT_MATCH_ITERATE(e, check_match, name, &e->ip, e->comefrom, &j);
 	if (ret)
-		return ret;
+		goto cleanup_matches;
+
+	ret = check_target(e, name);
+	if (ret)
+		goto cleanup_matches;
 
-	return check_target(e, name);
+	(*i)++;
+	return 0;
+
+ cleanup_matches:
+	IPT_MATCH_ITERATE(e, cleanup_match, &j);
+	return ret;
 }
 
 static int
@@ -1673,10 +1711,17 @@ translate_compat_table(const char *name,
 	if (!mark_source_chains(newinfo, valid_hooks, entry1))
 		goto free_newinfo;
 
+	i = 0;
 	ret = IPT_ENTRY_ITERATE(entry1, newinfo->size, compat_check_entry,
-									name);
-	if (ret)
-		goto free_newinfo;
+								name, &i);
+	if (ret) {
+		j -= i;
+		IPT_ENTRY_ITERATE_CONTINUE(entry1, newinfo->size, i,
+						compat_release_entry, &j);
+		IPT_ENTRY_ITERATE(entry1, newinfo->size, cleanup_entry, &i);
+		xt_free_table_info(newinfo);
+		return ret;
+	}
 
 	/* And one copy for every other CPU */
 	for_each_possible_cpu(i)
@@ -1691,7 +1736,7 @@ translate_compat_table(const char *name,
 free_newinfo:
 	xt_free_table_info(newinfo);
 out:
-	IPT_ENTRY_ITERATE(entry0, total_size, cleanup_entry, &j);
+	IPT_ENTRY_ITERATE(entry0, total_size, compat_release_entry, &j);
 	return ret;
 out_unlock:
 	compat_flush_offsets();

[NETFILTER 03/03]: nf_conntrack_amanda: fix textsearch_prepare() error check

[Patrick McHardy]

[NETFILTER]: nf_conntrack_amanda: fix textsearch_prepare() error check

The return value from textsearch_prepare() needs to be checked
by IS_ERR(). Because it returns error code as a pointer.

Cc: "Brian J. Murrell" <netfilter@interlinx.bc.ca>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 82224978c778e6e62e9d8d6f03e64d70bc494af4
tree 3107bf5a8fea7361cb0f4abec3ec0877b429d166
parent b14c27ef9486854969ae471aa5818b1e1352a0d7
author Akinobu Mita <akinobu.mita@gmail.com> Tue, 05 Jun 2007 15:33:04 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 05 Jun 2007 15:33:04 +0200

 net/netfilter/nf_conntrack_amanda.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index b8869ea..0568f2e 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -208,13 +208,14 @@ static int __init nf_conntrack_amanda_init(void)
 {
 	int ret, i;
 
-	ret = -ENOMEM;
 	for (i = 0; i < ARRAY_SIZE(search); i++) {
 		search[i].ts = textsearch_prepare(ts_algo, search[i].string,
 						  search[i].len,
 						  GFP_KERNEL, TS_AUTOLOAD);
-		if (search[i].ts == NULL)
+		if (IS_ERR(search[i].ts)) {
+			ret = PTR_ERR(search[i].ts);
 			goto err1;
+		}
 	}
 	ret = nf_conntrack_helper_register(&amanda_helper[0]);
 	if (ret < 0)
@@ -227,10 +228,9 @@ static int __init nf_conntrack_amanda_init(void)
 err2:
 	nf_conntrack_helper_unregister(&amanda_helper[0]);
 err1:
-	for (; i >= 0; i--) {
-		if (search[i].ts)
-			textsearch_destroy(search[i].ts);
-	}
+	while (--i >= 0)
+		textsearch_destroy(search[i].ts);
+
 	return ret;
 }
 

Re: [NETFILTER 01/03]: nf_conntrack: fix helper module unload races

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue,  5 Jun 2007 15:35:10 +0200 (MEST)

> [NETFILTER]: nf_conntrack: fix helper module unload races
> 
> When a helper module is unloaded all conntracks refering to it have their
> helper pointer NULLed out, leading to lots of races. In most places this
> can be fixed by proper use of RCU (they do already check for != NULL,
> but in a racy way), additionally nf_conntrack_expect_related needs to
> bail out when no helper is present.
> 
> Also remove two paranoid BUG_ONs in nf_conntrack_proto_gre that are racy
> and not worth fixing.
> 
> Signed-off-by: Patrick McHarrdy <kaber@trash.net>

Applied.

Re: [NETFILTER 02/03]: ip_tables: fix compat related crash

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue,  5 Jun 2007 15:35:11 +0200 (MEST)

> [NETFILTER]: ip_tables: fix compat related crash
> 
> check_compat_entry_size_and_hooks iterates over the matches and calls
> compat_check_calc_match, which loads the match and calculates the
> compat offsets, but unlike the non-compat version, doesn't call
> ->checkentry yet. On error however it calls cleanup_matches, which in
> turn calls ->destroy, which can result in crashes if the destroy
> function (validly) expects to only get called after the checkentry
> function.
> 
> Add a compat_release_match function that only drops the module reference
> on error and rename compat_check_calc_match to compat_find_calc_match to
> reflect the fact that it doesn't call the checkentry function.
> 
> Reported by Jan Engelhardt <jengelh@linux01.gwdg.de>
> 
> Signed-off-by: Dmitry Mishin <dim@openvz.org>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

Re: [NETFILTER 03/03]: nf_conntrack_amanda: fix textsearch_prepare() error check

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue,  5 Jun 2007 15:35:12 +0200 (MEST)

> [NETFILTER]: nf_conntrack_amanda: fix textsearch_prepare() error check
> 
> The return value from textsearch_prepare() needs to be checked
> by IS_ERR(). Because it returns error code as a pointer.
> 
> Cc: "Brian J. Murrell" <netfilter@interlinx.bc.ca>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Also applied, thanks a lot Patrick.

Re: [NETFILTER 01/03]: nf_conntrack: fix helper module unload races

[Yasuyuki KOZAKAI]

Hi, Patrick,

From: Patrick McHardy <kaber@trash.net>
Date: Tue,  5 Jun 2007 15:35:10 +0200 (MEST)

> [NETFILTER]: nf_conntrack: fix helper module unload races

nfct_help(ct) also needs to be protected because of the race between
nfctnetlink and helper. Sorry, I did not test the following patch because
I don't have SMP machine.

You might dislike this.  Other idea is to use call_rcu() in
nfctnetlink_change_helper(), but I need to study the way to use it more.


[NETFILTER]: nf_conntrack: Fixes race on changing helper

nfctnetlink may change the helper assigned to a conntrack and clear
private area in the conntrack. To fix the race, this exports spin locks
of helpers and nfctnetlink uses them. Some helpers do not need to grab
spin lock, because they do not store any state in the private area.

Other staff does not need to grab spin lock.
- unhelp() assigns NULL to conntrack, but it does not touch the private
  area.
- init_conntrack(), nf_conntrack_alter_reply(), and
  nfctnetlink_create_conntrack() assign a helper to a conntrack,
  but the conntrack is unconfirmed. No competitor exists.

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>
---
 include/net/netfilter/nf_conntrack_helper.h    |    5 +++++
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   21 +++++++++++++++++----
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |   16 +++++++++++++---
 net/netfilter/nf_conntrack_ftp.c               |    3 +--
 net/netfilter/nf_conntrack_h323_main.c         |   20 ++++++--------------
 net/netfilter/nf_conntrack_irc.c               |    3 +--
 net/netfilter/nf_conntrack_netlink.c           |    6 ++++++
 net/netfilter/nf_conntrack_pptp.c              |    4 +---
 net/netfilter/nf_conntrack_sane.c              |    3 +--
 9 files changed, 51 insertions(+), 30 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 8c72ac9..5a2b811 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -10,6 +10,7 @@
 #ifndef _NF_CONNTRACK_HELPER_H
 #define _NF_CONNTRACK_HELPER_H
 #include <net/netfilter/nf_conntrack.h>
+#include <linux/spinlock_types.h>
 
 struct module;
 
@@ -37,6 +38,10 @@ struct nf_conntrack_helper
 	void (*destroy)(struct nf_conn *ct);
 
 	int (*to_nfattr)(struct sk_buff *skb, const struct nf_conn *ct);
+
+	/* This protects the helper specific shared resource and
+	   the private area in a confirmed conntrack */
+	spinlock_t *lock;
 };
 
 extern struct nf_conntrack_helper *
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 6dc72a8..648b7fd 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -134,6 +134,7 @@ static unsigned int ipv4_conntrack_help(
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn_help *help;
 	struct nf_conntrack_helper *helper;
+	int ret;
 
 	/* This is where we call the helper: as the packet goes out. */
 	ct = nf_ct_get(*pskb, &ctinfo);
@@ -143,12 +144,24 @@ static unsigned int ipv4_conntrack_help(
 	help = nfct_help(ct);
 	if (!help)
 		return NF_ACCEPT;
+
+	if (helper->lock)
+		spin_lock_bh(helper->lock);
+
 	/* rcu_read_lock()ed by nf_hook_slow */
 	helper = rcu_dereference(help->helper);
-	if (!helper)
-		return NF_ACCEPT;
-	return helper->help(pskb, skb_network_offset(*pskb) + ip_hdrlen(*pskb),
-			    ct, ctinfo);
+	if (!helper) {
+		ret = NF_ACCEPT;
+		goto out;
+	}
+
+	ret = helper->help(pskb, skb_network_offset(*pskb) + ip_hdrlen(*pskb),
+			   ct, ctinfo);
+out:
+	if (helper->lock)
+		spin_unlock_bh(helper->lock);
+
+	return ret;
 }
 
 static unsigned int ipv4_conntrack_defrag(unsigned int hooknum,
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index 1b1797f..0fa34a2 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -162,7 +162,8 @@ static unsigned int ipv6_confirm(unsigne
 	struct nf_conn_help *help;
 	struct nf_conntrack_helper *helper;
 	enum ip_conntrack_info ctinfo;
-	unsigned int ret, protoff;
+	unsigned int ret = NF_ACCEPT;
+	unsigned int protoff;
 	unsigned int extoff = (u8 *)(ipv6_hdr(*pskb) + 1) - (*pskb)->data;
 	unsigned char pnum = ipv6_hdr(*pskb)->nexthdr;
 
@@ -175,19 +176,28 @@ static unsigned int ipv6_confirm(unsigne
 	help = nfct_help(ct);
 	if (!help)
 		goto out;
+
+	if (helper->lock)
+		spin_lock_bh(helper->lock);
+
 	/* rcu_read_lock()ed by nf_hook_slow */
 	helper = rcu_dereference(help->helper);
 	if (!helper)
-		goto out;
+		goto out_unlock;
 
 	protoff = nf_ct_ipv6_skip_exthdr(*pskb, extoff, &pnum,
 					 (*pskb)->len - extoff);
 	if (protoff > (*pskb)->len || pnum == NEXTHDR_FRAGMENT) {
 		DEBUGP("proto header not found\n");
-		return NF_ACCEPT;
+		goto out_unlock;
 	}
 
 	ret = helper->help(pskb, protoff, ct, ctinfo);
+
+out_unlock:
+	if (helper->lock)
+		spin_unlock_bh(helper->lock);
+
 	if (ret != NF_ACCEPT)
 		return ret;
 out:
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 82db2aa..e9700f6 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -389,7 +389,6 @@ static int help(struct sk_buff **pskb,
 	}
 	datalen = (*pskb)->len - dataoff;
 
-	spin_lock_bh(&nf_ftp_lock);
 	fb_ptr = skb_header_pointer(*pskb, dataoff, datalen, ftp_buffer);
 	BUG_ON(fb_ptr == NULL);
 
@@ -538,7 +537,6 @@ out_update_nl:
 	if (ends_in_nl)
 		update_nl_seq(seq, ct_ftp_info, dir, *pskb);
  out:
-	spin_unlock_bh(&nf_ftp_lock);
 	return ret;
 }
 
@@ -591,6 +589,7 @@ static int __init nf_conntrack_ftp_init(
 			ftp[i][j].timeout = 5 * 60;	/* 5 Minutes */
 			ftp[i][j].me = THIS_MODULE;
 			ftp[i][j].help = help;
+			ftp[i][j].lock = &nf_ftp_lock;
 			tmpname = &ftp_names[i][j][0];
 			if (ports[i] == FTP_PORT)
 				sprintf(tmpname, "ftp");
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index a1b95ac..38ceda4 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -584,8 +584,6 @@ static int h245_help(struct sk_buff **ps
 	}
 	DEBUGP("nf_ct_h245: skblen = %u\n", (*pskb)->len);
 
-	spin_lock_bh(&nf_h323_lock);
-
 	/* Process each TPKT */
 	while (get_tpkt_data(pskb, protoff, ct, ctinfo,
 			     &data, &datalen, &dataoff)) {
@@ -609,11 +607,9 @@ static int h245_help(struct sk_buff **ps
 			goto drop;
 	}
 
-	spin_unlock_bh(&nf_h323_lock);
 	return NF_ACCEPT;
 
       drop:
-	spin_unlock_bh(&nf_h323_lock);
 	if (net_ratelimit())
 		printk("nf_ct_h245: packet dropped\n");
 	return NF_DROP;
@@ -1128,8 +1124,6 @@ static int q931_help(struct sk_buff **ps
 	}
 	DEBUGP("nf_ct_q931: skblen = %u\n", (*pskb)->len);
 
-	spin_lock_bh(&nf_h323_lock);
-
 	/* Process each TPKT */
 	while (get_tpkt_data(pskb, protoff, ct, ctinfo,
 			     &data, &datalen, &dataoff)) {
@@ -1152,11 +1146,9 @@ static int q931_help(struct sk_buff **ps
 			goto drop;
 	}
 
-	spin_unlock_bh(&nf_h323_lock);
 	return NF_ACCEPT;
 
       drop:
-	spin_unlock_bh(&nf_h323_lock);
 	if (net_ratelimit())
 		printk("nf_ct_q931: packet dropped\n");
 	return NF_DROP;
@@ -1176,7 +1168,8 @@ static struct nf_conntrack_helper nf_con
 		.mask.src.l3num		= 0xFFFF,
 		.mask.src.u.tcp.port	= __constant_htons(0xFFFF),
 		.mask.dst.protonum	= 0xFF,
-		.help			= q931_help
+		.help			= q931_help,
+		.lock			= &nf_h323_lock,
 	},
 	{
 		.name			= "Q.931",
@@ -1190,7 +1183,8 @@ static struct nf_conntrack_helper nf_con
 		.mask.src.l3num		= 0xFFFF,
 		.mask.src.u.tcp.port	= __constant_htons(0xFFFF),
 		.mask.dst.protonum	= 0xFF,
-		.help			= q931_help
+		.help			= q931_help,
+		.lock			= &nf_h323_lock,
 	},
 };
 
@@ -1708,8 +1702,6 @@ static int ras_help(struct sk_buff **psk
 
 	DEBUGP("nf_ct_ras: skblen = %u\n", (*pskb)->len);
 
-	spin_lock_bh(&nf_h323_lock);
-
 	/* Get UDP data */
 	data = get_udp_data(pskb, protoff, &datalen);
 	if (data == NULL)
@@ -1732,11 +1724,9 @@ static int ras_help(struct sk_buff **psk
 		goto drop;
 
       accept:
-	spin_unlock_bh(&nf_h323_lock);
 	return NF_ACCEPT;
 
       drop:
-	spin_unlock_bh(&nf_h323_lock);
 	if (net_ratelimit())
 		printk("nf_ct_ras: packet dropped\n");
 	return NF_DROP;
@@ -1756,6 +1746,7 @@ static struct nf_conntrack_helper nf_con
 		.mask.src.u.udp.port	= __constant_htons(0xFFFF),
 		.mask.dst.protonum	= 0xFF,
 		.help			= ras_help,
+		.lock			= &nf_h323_lock,
 	},
 	{
 		.name			= "RAS",
@@ -1769,6 +1760,7 @@ static struct nf_conntrack_helper nf_con
 		.mask.src.u.udp.port	= __constant_htons(0xFFFF),
 		.mask.dst.protonum	= 0xFF,
 		.help			= ras_help,
+		.lock			= &nf_h323_lock,
 	},
 };
 
diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 43ccd0e..2e82fbc 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -130,7 +130,6 @@ static int help(struct sk_buff **pskb, u
 	if (dataoff >= (*pskb)->len)
 		return NF_ACCEPT;
 
-	spin_lock_bh(&irc_buffer_lock);
 	ib_ptr = skb_header_pointer(*pskb, dataoff, (*pskb)->len - dataoff,
 				    irc_buffer);
 	BUG_ON(ib_ptr == NULL);
@@ -208,7 +207,6 @@ static int help(struct sk_buff **pskb, u
 		}
 	}
  out:
-	spin_unlock_bh(&irc_buffer_lock);
 	return ret;
 }
 
@@ -246,6 +244,7 @@ static int __init nf_conntrack_irc_init(
 		irc[i].timeout = dcc_timeout;
 		irc[i].me = THIS_MODULE;
 		irc[i].help = help;
+		irc[i].lock = &irc_buffer_lock;
 
 		tmpname = &irc_names[i][0];
 		if (ports[i] == IRC_PORT)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 3f73327..f604fff 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -872,10 +872,16 @@ ctnetlink_change_helper(struct nf_conn *
 		/* we had a helper before ... */
 		nf_ct_remove_expectations(ct);
 
+	if (help->helper->lock)
+		spin_lock_bh(help->helper->lock);
+
 	/* need to zero data of old helper */
 	memset(&help->help, 0, sizeof(help->help));
 	rcu_assign_pointer(help->helper, helper);
 
+	if (help->helper->lock)
+		spin_unlock_bh(help->helper->lock);
+
 	return 0;
 }
 
diff --git a/net/netfilter/nf_conntrack_pptp.c b/net/netfilter/nf_conntrack_pptp.c
index 115bcb5..08825f8 100644
--- a/net/netfilter/nf_conntrack_pptp.c
+++ b/net/netfilter/nf_conntrack_pptp.c
@@ -557,8 +557,6 @@ conntrack_pptp_help(struct sk_buff **psk
 	oldsstate = info->sstate;
 	oldcstate = info->cstate;
 
-	spin_lock_bh(&nf_pptp_lock);
-
 	/* FIXME: We just blindly assume that the control connection is always
 	 * established from PNS->PAC.  However, RFC makes no guarantee */
 	if (dir == IP_CT_DIR_ORIGINAL)
@@ -571,7 +569,6 @@ conntrack_pptp_help(struct sk_buff **psk
 				       ctinfo);
 	DEBUGP("sstate: %d->%d, cstate: %d->%d\n",
 		oldsstate, info->sstate, oldcstate, info->cstate);
-	spin_unlock_bh(&nf_pptp_lock);
 
 	return ret;
 }
@@ -590,6 +587,7 @@ static struct nf_conntrack_helper pptp _
 	.mask.dst.protonum	= 0xff,
 	.help			= conntrack_pptp_help,
 	.destroy		= pptp_destroy_siblings,
+	.lock			= &nf_pptp_lock,
 };
 
 static int __init nf_conntrack_pptp_init(void)
diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index eb2d1dc..bcd5238 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -97,7 +97,6 @@ static int help(struct sk_buff **pskb,
 
 	datalen = (*pskb)->len - dataoff;
 
-	spin_lock_bh(&nf_sane_lock);
 	sb_ptr = skb_header_pointer(*pskb, dataoff, datalen, sane_buffer);
 	BUG_ON(sb_ptr == NULL);
 
@@ -164,7 +163,6 @@ static int help(struct sk_buff **pskb,
 	nf_conntrack_expect_put(exp);
 
 out:
-	spin_unlock_bh(&nf_sane_lock);
 	return ret;
 }
 
@@ -214,6 +212,7 @@ static int __init nf_conntrack_sane_init
 			sane[i][j].timeout = 5 * 60;	/* 5 Minutes */
 			sane[i][j].me = THIS_MODULE;
 			sane[i][j].help = help;
+			sane[i][j].lock = &nf_sane_lock;
 			tmpname = &sane_names[i][j][0];
 			if (ports[i] == SANE_PORT)
 				sprintf(tmpname, "sane");
-- 
1.4.4

Re: [NETFILTER 01/03]: nf_conntrack: fix helper module unload races

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> nfct_help(ct) also needs to be protected because of the race between
> nfctnetlink and helper. Sorry, I did not test the following patch because
> I don't have SMP machine.
> 
> You might dislike this.  Other idea is to use call_rcu() in
> nfctnetlink_change_helper(), but I need to study the way to use it more.
> 
> 
> [NETFILTER]: nf_conntrack: Fixes race on changing helper
> 
> nfctnetlink may change the helper assigned to a conntrack and clear
> private area in the conntrack. To fix the race, this exports spin locks
> of helpers and nfctnetlink uses them. Some helpers do not need to grab
> spin lock, because they do not store any state in the private area.
> 
> Other staff does not need to grab spin lock.
> - unhelp() assigns NULL to conntrack, but it does not touch the private
>   area.
> - init_conntrack(), nf_conntrack_alter_reply(), and
>   nfctnetlink_create_conntrack() assign a helper to a conntrack,
>   but the conntrack is unconfirmed. No competitor exists.


The fix looks correct, but considering the other problems that changing
helpers cause for ct_extend I'm wondering whether we could restrict the
helper change operation to conntracks that don't already have a helper
assigned. I can't imagine any use for changing helpers of a connection
that already has one assigned and it will always be racy anyway.
Pablo, are you aware of anything using/needing this feature?

[NETFILTER]: nfctnetlink: Don't allow to change helper (Was: Re: [NETFILTER 01/03]: nf_conntrack: fix helper module unload races)

[Yasuyuki KOZAKAI]

Hi, Pablo,

What do you think about this ?

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 18 Jun 2007 14:29:27 +0200

> > [NETFILTER]: nf_conntrack: Fixes race on changing helper
> > 
> > nfctnetlink may change the helper assigned to a conntrack and clear
> > private area in the conntrack. To fix the race, this exports spin locks
> > of helpers and nfctnetlink uses them. Some helpers do not need to grab
> > spin lock, because they do not store any state in the private area.
> > 
> > Other staff does not need to grab spin lock.
> > - unhelp() assigns NULL to conntrack, but it does not touch the private
> >   area.
> > - init_conntrack(), nf_conntrack_alter_reply(), and
> >   nfctnetlink_create_conntrack() assign a helper to a conntrack,
> >   but the conntrack is unconfirmed. No competitor exists.
> 
> 
> The fix looks correct, but considering the other problems that changing
> helpers cause for ct_extend I'm wondering whether we could restrict the
> helper change operation to conntracks that don't already have a helper
> assigned. I can't imagine any use for changing helpers of a connection
> that already has one assigned and it will always be racy anyway.
> Pablo, are you aware of anything using/needing this feature?




[NETFILTER]: nfctnetlink: Don't allow to change helper

There is no realistic situation to change helper (Who wants IRC helper to
track FTP traffic ?). Moreover, if we want to do that, we need to fix race
issue by nfctnetlink and running helper. That will add overhead to packet
processing. It wouldn't pay. So this rejects the request to change
helper. The requests to add or remove helper are accepted as ever.

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>
---
 net/netfilter/nf_conntrack_netlink.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 3f73327..d0fe3d7 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -869,8 +869,7 @@ ctnetlink_change_helper(struct nf_conn *ct, struct nfattr *cda[])
 		return 0;
 
 	if (help->helper)
-		/* we had a helper before ... */
-		nf_ct_remove_expectations(ct);
+		return -EBUSY;
 
 	/* need to zero data of old helper */
 	memset(&help->help, 0, sizeof(help->help));
-- 
1.5.2.2

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
when loading the NAT module, an invalid return code in ctnetlink and a possible
NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
-stable once its upstream.

Please apply, thanks.


 include/net/netfilter/ipv4/nf_conntrack_ipv4.h |    2 ++
 net/ipv4/netfilter/ipt_recent.c                |    7 ++++++-
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    6 ++++++
 net/ipv4/netfilter/nf_nat_standalone.c         |    2 +-
 net/netfilter/nf_conntrack_netlink.c           |   17 +++++++++--------
 5 files changed, 24 insertions(+), 10 deletions(-)

Jesper Juhl (1):
      [NETFILTER]: ipt_recent: avoid a possible NULL pointer deref in recent_seq_open()

Pablo Neira Ayuso (1):
      [NETFILTER]: ctnetlink: return EEXIST instead of EINVAL for existing nat'ed conntracks

Patrick McHardy (1):
      [NETFILTER]: nf_nat: add symbolic dependency on IPv4 conntrack

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon,  6 Aug 2007 15:29:03 +0200 (MEST)

> these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
> when loading the NAT module, an invalid return code in ctnetlink and a possible
> NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
> -stable once its upstream.
> 
> Please apply, thanks.

Applied, thanks Patrick.

I really wish those dependencies could be worked out in a nicer
way than calling NULL functions in the needed module.

Re: [NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon,  6 Aug 2007 15:29:03 +0200 (MEST)
>
>   
>> these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
>> when loading the NAT module, an invalid return code in ctnetlink and a possible
>> NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
>> -stable once its upstream.
>>
>> Please apply, thanks.
>>     
>
> Applied, thanks Patrick.
>
> I really wish those dependencies could be worked out in a nicer
> way than calling NULL functions in the needed module.
>   

Its not very pretty, I agree. In this case we could have used
indirect dependencies and request_module, but I actually prefer
the symbol dependency because its visible in lsmod, which makes
it easier to figure out what needs to be unloaded first to
remove a module.

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these three patches fix a nf_nat memset error, leading to misbehaviour
when unloading and reloading the NAT module, a regression from the
bridge netfilter deferred hook removal causing double invocation of the
POSTROUTING hook for packets forwarded between two bridge devices and
consolidate the nf_sockopt code. I'll push the memset and bridge fixes
to -stable once they hit Linus' tree.

Please apply, thanks.


 net/bridge/br_netfilter.c        |    3 +
 net/ipv4/netfilter/nf_nat_core.c |    2 +-
 net/netfilter/nf_sockopt.c       |  106 ++++++++++++++++----------------------
 3 files changed, 48 insertions(+), 63 deletions(-)

Li Zefan (1):
      [NETFILTER]: nf_nat: fix memset error

Patrick McHardy (1):
      [NETFILTER]: bridge: fix double POSTROUTING hook invocation

Pavel Emelyanov (1):
      [NETFILTER]: Consolidate nf_sockopt and compat_nf_sockopt

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Herbert,

these patches for 2.6.24 fix a number of netfilter bugs: a refcount leak in a
CONNMARK and CONNSECMARK error path, a network triggerable WARN_ON in the
IPv6 TCPMSS target and an endless loop caused by passing a zero-length pattern
to the string match.

Please apply, thanks.


 lib/textsearch.c               |    8 ++++++--
 net/netfilter/xt_CONNMARK.c    |   10 +++++-----
 net/netfilter/xt_CONNSECMARK.c |   10 +++++-----
 net/netfilter/xt_TCPMSS.c      |    4 +---
 4 files changed, 17 insertions(+), 15 deletions(-)

Jan Engelhardt (1):
      [NETFILTER]: fix forgotten module release in xt_CONNMARK and xt_CONNSECMARK

Pablo Neira Ayuso (1):
      [TEXTSEARCH]: Do not allow zero length patterns in the textsearch infrastructure

Patrick McHardy (1):
      [NETFILTER]: xt_TCPMSS: remove network triggerable WARN_ON

Re: [NETFILTER 00/03]: Netfilter fixes

[Herbert Xu]

On Fri, Nov 30, 2007 at 12:57:12AM +0100, Patrick McHardy wrote:
> 
> these patches for 2.6.24 fix a number of netfilter bugs: a refcount leak in a
> CONNMARK and CONNSECMARK error path, a network triggerable WARN_ON in the
> IPv6 TCPMSS target and an endless loop caused by passing a zero-length pattern
> to the string match.
> 
> Please apply, thanks.

All applied.  Thanks a lot Patrick.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these three patches fix (again) skb_over_panic caused by netfilter queueing,
a namespace leak when reading /proc/net/xxx_tables_names and incorrect error
handling in the TCPOPTSTRIP target.

Please apply, thanks.


 net/ipv4/netfilter/ip_queue.c   |    5 ++---
 net/ipv6/netfilter/ip6_queue.c  |    5 ++---
 net/netfilter/nfnetlink_queue.c |    5 ++---
 net/netfilter/x_tables.c        |    2 +-
 net/netfilter/xt_TCPOPTSTRIP.c  |    2 +-
 5 files changed, 8 insertions(+), 11 deletions(-)

Arnaud Ebalard (1):
      [NETFILTER]: {nfnetlink,ip,ip6}_queue: fix skb_over_panic when enlarging packets

Pavel Emelyanov (1):
      [NETFILTER]: x_tables: fix net namespace leak when reading /proc/net/xxx_tables_names

Roel Kluin (1):
      [NETFILTER]: xt_TCPOPTSTRIP: signed tcphoff for	ipv6_skip_exthdr() retval

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 29 Apr 2008 00:06:40 +0200 (MEST)

> these three patches fix (again) skb_over_panic caused by netfilter queueing,
> a namespace leak when reading /proc/net/xxx_tables_names and incorrect error
> handling in the TCPOPTSTRIP target.
> 
> Please apply, thanks.

All 3 patches applied, thanks Patrick.