[PATCH 0/3] netfilter fixes for 3.4-rc1

[PATCH 0/3] netfilter fixes for 3.4-rc1

[pablo]

From: Pablo Neira Ayuso <pablo@netfilter.org>

[ This is a resent including netdev mailing list, sorry I forgot to include it ]

Hi David,

The following patches contain a couple of small fixes for Netfilter:

* One compilation warning fix for xt_LOG from Jan Beulich.

* Fix compilation issues of user-land ipset with kernel header file
  from Jan Engelhardt via Jozsef Kadlecsik.

* Fix one compilation warning in the recent changes done in xt_CT to
  support the extended cttimeout infrastracture, spotted by Eric
  Dumazet.

You can pull them from:

git://1984.lsi.us.es/net master

Thanks!

Jan Beulich (1):
  netfilter: xt_LOG: don't use xchg() for simple assignment

Jan Engelhardt (1):
  netfilter: ipset: avoid use of kernel-only types

Pablo Neira Ayuso (1):
  netfilter: xt_CT: remove a compile warning

 include/linux/netfilter/xt_set.h |    4 ++--
 include/net/netfilter/xt_log.h   |    2 +-
 net/netfilter/xt_CT.c            |    2 ++
 3 files changed, 5 insertions(+), 3 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] netfilter: xt_LOG: don't use xchg() for simple assignment

[pablo]

From: Jan Beulich <JBeulich@suse.com>

At least on ia64 the (bogus) use of xchg() here results in the compiler
warning about an unused expression result. As only an assignment is
intended here, convert it to such.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/xt_log.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/netfilter/xt_log.h b/include/net/netfilter/xt_log.h
index 7e1544e..9d9756c 100644
--- a/include/net/netfilter/xt_log.h
+++ b/include/net/netfilter/xt_log.h
@@ -47,7 +47,7 @@ static void sb_close(struct sbuff *m)
 	if (likely(m != &emergency))
 		kfree(m);
 	else {
-		xchg(&emergency_ptr, m);
+		emergency_ptr = m;
 		local_bh_enable();
 	}
 }
-- 
1.7.2.5

[PATCH 2/3] netfilter: ipset: avoid use of kernel-only types

[pablo]

From: Jan Engelhardt <jengelh@medozas.de>

When using the xt_set.h header in userspace, one will get these gcc
reports:

ipset/ip_set.h:184:1: error: unknown type name "u16"
In file included from libxt_SET.c:21:0:
netfilter/xt_set.h:61:2: error: unknown type name "u32"
netfilter/xt_set.h:62:2: error: unknown type name "u32"

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netfilter/xt_set.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/netfilter/xt_set.h b/include/linux/netfilter/xt_set.h
index c0405ac..e3a9978 100644
--- a/include/linux/netfilter/xt_set.h
+++ b/include/linux/netfilter/xt_set.h
@@ -58,8 +58,8 @@ struct xt_set_info_target_v1 {
 struct xt_set_info_target_v2 {
 	struct xt_set_info add_set;
 	struct xt_set_info del_set;
-	u32 flags;
-	u32 timeout;
+	__u32 flags;
+	__u32 timeout;
 };
 
 #endif /*_XT_SET_H*/
-- 
1.7.2.5

[PATCH 3/3] netfilter: xt_CT: remove a compile warning

[pablo]

From: Pablo Neira Ayuso <pablo@netfilter.org>

If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning :

  CC [M]  net/netfilter/xt_CT.o
net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’:
net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 0c8e438..138b75e 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -281,8 +281,10 @@ out:
 	info->ct = ct;
 	return 0;
 
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 err4:
 	rcu_read_unlock();
+#endif
 err3:
 	nf_conntrack_free(ct);
 err2:
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Tetsuo Handa]

pablo@netfilter.org:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning :
> 
>   CC [M]  net/netfilter/xt_CT.o
> net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’:
> net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/xt_CT.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
> index 0c8e438..138b75e 100644
> --- a/net/netfilter/xt_CT.c
> +++ b/net/netfilter/xt_CT.c
> @@ -281,8 +281,10 @@ out:
>  	info->ct = ct;
>  	return 0;
>  
> +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
>  err4:
>  	rcu_read_unlock();
> +#endif
>  err3:
>  	nf_conntrack_free(ct);
>  err2:
> -- 
> 1.7.2.5

Looking at that function:

216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
217         if (info->timeout) {
218                 typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
219                 struct ctnl_timeout *timeout;
220                 struct nf_conn_timeout *timeout_ext;
221 
222                 rcu_read_lock();
223                 timeout_find_get =
224                         rcu_dereference(nf_ct_timeout_find_get_hook);
225 
226                 if (timeout_find_get) {
227                         const struct ipt_entry *e = par->entryinfo;
228                         struct nf_conntrack_l4proto *l4proto;
229 
230                         if (e->ip.invflags & IPT_INV_PROTO) {
231                                 ret = -EINVAL;
232                                 pr_info("You cannot use inversion on "
233                                          "L4 protocol\n");
234                                 goto err4;
235                         }
236                         timeout = timeout_find_get(info->timeout);
237                         if (timeout == NULL) {
238                                 ret = -ENOENT;
239                                 pr_info("No such timeout policy \"%s\"\n",
240                                         info->timeout);
241                                 goto err4;
242                         }
243                         if (timeout->l3num != par->family) {
244                                 ret = -EINVAL;
245                                 pr_info("Timeout policy `%s' can only be "
246                                         "used by L3 protocol number %d\n",
247                                         info->timeout, timeout->l3num);
248                                 goto err4;
249                         }
250                         /* Make sure the timeout policy matches any existing
251                          * protocol tracker, otherwise default to generic.
252                          */
253                         l4proto = __nf_ct_l4proto_find(par->family,
254                                                        e->ip.proto);
255                         if (timeout->l4proto->l4proto != l4proto->l4proto) {
256                                 ret = -EINVAL;
257                                 pr_info("Timeout policy `%s' can only be "
258                                         "used by L4 protocol number %d\n",
259                                         info->timeout,
260                                         timeout->l4proto->l4proto);
261                                 goto err4;
262                         }
263                         timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
264                                                             GFP_KERNEL);
                            We are under rcu_read_lock() here.

265                         if (timeout_ext == NULL) {
266                                 ret = -ENOMEM;
267                                 goto err4;
268                         }
269                 } else {
270                         ret = -ENOENT;
271                         pr_info("Timeout policy base is empty\n");
272                         goto err4;
273                 }
274                 rcu_read_unlock();
275         }
276 #endif

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

On Tue, Apr 03, 2012 at 07:27:50PM +0900, Tetsuo Handa wrote:
> pablo@netfilter.org:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning :
> > 
> >   CC [M]  net/netfilter/xt_CT.o
> > net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’:
> > net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used
> > 
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/xt_CT.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
> > index 0c8e438..138b75e 100644
> > --- a/net/netfilter/xt_CT.c
> > +++ b/net/netfilter/xt_CT.c
> > @@ -281,8 +281,10 @@ out:
> >  	info->ct = ct;
> >  	return 0;
> >  
> > +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> >  err4:
> >  	rcu_read_unlock();
> > +#endif
> >  err3:
> >  	nf_conntrack_free(ct);
> >  err2:
> > -- 
> > 1.7.2.5
> 
> Looking at that function:
> 
> 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> 217         if (info->timeout) {
> 218                 typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
> 219                 struct ctnl_timeout *timeout;
> 220                 struct nf_conn_timeout *timeout_ext;
> 221 
> 222                 rcu_read_lock();
> 223                 timeout_find_get =
> 224                         rcu_dereference(nf_ct_timeout_find_get_hook);
> 225 
> 226                 if (timeout_find_get) {
> 227                         const struct ipt_entry *e = par->entryinfo;
> 228                         struct nf_conntrack_l4proto *l4proto;
> 229 
> 230                         if (e->ip.invflags & IPT_INV_PROTO) {
> 231                                 ret = -EINVAL;
> 232                                 pr_info("You cannot use inversion on "
> 233                                          "L4 protocol\n");
> 234                                 goto err4;
> 235                         }
> 236                         timeout = timeout_find_get(info->timeout);
> 237                         if (timeout == NULL) {
> 238                                 ret = -ENOENT;
> 239                                 pr_info("No such timeout policy \"%s\"\n",
> 240                                         info->timeout);
> 241                                 goto err4;
> 242                         }
> 243                         if (timeout->l3num != par->family) {
> 244                                 ret = -EINVAL;
> 245                                 pr_info("Timeout policy `%s' can only be "
> 246                                         "used by L3 protocol number %d\n",
> 247                                         info->timeout, timeout->l3num);
> 248                                 goto err4;
> 249                         }
> 250                         /* Make sure the timeout policy matches any existing
> 251                          * protocol tracker, otherwise default to generic.
> 252                          */
> 253                         l4proto = __nf_ct_l4proto_find(par->family,
> 254                                                        e->ip.proto);
> 255                         if (timeout->l4proto->l4proto != l4proto->l4proto) {
> 256                                 ret = -EINVAL;
> 257                                 pr_info("Timeout policy `%s' can only be "
> 258                                         "used by L4 protocol number %d\n",
> 259                                         info->timeout,
> 260                                         timeout->l4proto->l4proto);
> 261                                 goto err4;
> 262                         }
> 263                         timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
> 264                                                             GFP_KERNEL);
>                             We are under rcu_read_lock() here.

Good catch, that needs to be GFP_ATOMIC. I'll send a follow-up patch
for this.

Thanks.

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

On Tue, Apr 03, 2012 at 07:27:50PM +0900, Tetsuo Handa wrote:
> pablo@netfilter.org:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning :
> > 
> >   CC [M]  net/netfilter/xt_CT.o
> > net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’:
> > net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used
> > 
> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/xt_CT.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
> > index 0c8e438..138b75e 100644
> > --- a/net/netfilter/xt_CT.c
> > +++ b/net/netfilter/xt_CT.c
> > @@ -281,8 +281,10 @@ out:
> >  	info->ct = ct;
> >  	return 0;
> >  
> > +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> >  err4:
> >  	rcu_read_unlock();
> > +#endif
> >  err3:
> >  	nf_conntrack_free(ct);
> >  err2:
> > -- 
> > 1.7.2.5
> 
> Looking at that function:
> 
[...]
> 262                         }
> 263                         timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
> 264                                                             GFP_KERNEL);
>                             We are under rcu_read_lock() here.

David, can you also apply the following patch?

Thanks.

[-- Attachment #2: 0001-netfilter-xt_CT-allocation-has-to-be-GFP_ATOMIC-unde.patch --]
[-- Type: text/x-diff, Size: 890 bytes --]

>From e21e9622a46a60bd2b8dfb9791bb63dfdd4940d7 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 12:32:15 +0200
Subject: [PATCH] netfilter: xt_CT: allocation has to be GFP_ATOMIC under rcu_read_lock section

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 138b75e..4babb27 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -261,7 +261,7 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 				goto err4;
 			}
 			timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
-							    GFP_KERNEL);
+							    GFP_ATOMIC);
 			if (timeout_ext == NULL) {
 				ret = -ENOMEM;
 				goto err4;
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Tetsuo Handa]

One more question.

Tetsuo Handa wrote:
> 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> 217         if (info->timeout) {
> 218                 typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
> 219                 struct ctnl_timeout *timeout;
> 220                 struct nf_conn_timeout *timeout_ext;
> 221 
> 222                 rcu_read_lock();
> 223                 timeout_find_get =
> 224                         rcu_dereference(nf_ct_timeout_find_get_hook);
> 225 
> 226                 if (timeout_find_get) {

I assume timeout_find_get points to e.g. ctnl_timeout_find_get in
net/netfilter/nfnetlink_cttimeout.c . If yes,

> 227                         const struct ipt_entry *e = par->entryinfo;
> 228                         struct nf_conntrack_l4proto *l4proto;
> 229 
> 230                         if (e->ip.invflags & IPT_INV_PROTO) {
> 231                                 ret = -EINVAL;
> 232                                 pr_info("You cannot use inversion on "
> 233                                          "L4 protocol\n");
> 234                                 goto err4;
> 235                         }
> 236                         timeout = timeout_find_get(info->timeout);
> 237                         if (timeout == NULL) {
> 238                                 ret = -ENOENT;
> 239                                 pr_info("No such timeout policy \"%s\"\n",
> 240                                         info->timeout);
> 241                                 goto err4;
> 242                         }

I think "goto err4;" after successful timeout_find_get() wants e.g.
nf_ct_timeout_put_hook call (e.g. ctnl_timeout_put()).

> 243                         if (timeout->l3num != par->family) {
> 244                                 ret = -EINVAL;
> 245                                 pr_info("Timeout policy `%s' can only be "
> 246                                         "used by L3 protocol number %d\n",
> 247                                         info->timeout, timeout->l3num);
> 248                                 goto err4;
> 249                         }

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

On Tue, Apr 03, 2012 at 08:48:21PM +0900, Tetsuo Handa wrote:
> One more question.
> 
> Tetsuo Handa wrote:
> > 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> > 217         if (info->timeout) {
> > 218                 typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
> > 219                 struct ctnl_timeout *timeout;
> > 220                 struct nf_conn_timeout *timeout_ext;
> > 221 
> > 222                 rcu_read_lock();
> > 223                 timeout_find_get =
> > 224                         rcu_dereference(nf_ct_timeout_find_get_hook);
> > 225 
> > 226                 if (timeout_find_get) {
> 
> I assume timeout_find_get points to e.g. ctnl_timeout_find_get in
> net/netfilter/nfnetlink_cttimeout.c . If yes,
> 
> > 227                         const struct ipt_entry *e = par->entryinfo;
> > 228                         struct nf_conntrack_l4proto *l4proto;
> > 229 
> > 230                         if (e->ip.invflags & IPT_INV_PROTO) {
> > 231                                 ret = -EINVAL;
> > 232                                 pr_info("You cannot use inversion on "
> > 233                                          "L4 protocol\n");
> > 234                                 goto err4;
> > 235                         }
> > 236                         timeout = timeout_find_get(info->timeout);
> > 237                         if (timeout == NULL) {
> > 238                                 ret = -ENOENT;
> > 239                                 pr_info("No such timeout policy \"%s\"\n",
> > 240                                         info->timeout);
> > 241                                 goto err4;
> > 242                         }
> 
> I think "goto err4;" after successful timeout_find_get() wants e.g.
> nf_ct_timeout_put_hook call (e.g. ctnl_timeout_put()).

Indeed. The patch attached should fix this problem. Thanks.

[-- Attachment #2: 0001-netfilter-xt_CT-fix-missing-put-timeout-object-in-er.patch --]
[-- Type: text/x-diff, Size: 3112 bytes --]

>From a7ff65f786a38c6a612eb3b65ccdf9ea4c517503 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 14:00:18 +0200
Subject: [PATCH] netfilter: xt_CT: fix missing put timeout object in error path

The error path misses putting the timeout object. This patch adds
new function xt_ct_tg_timeout_put() to do that to avoid code
duplication.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |   44 +++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 4babb27..89126fc 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -150,6 +150,24 @@ err1:
 	return ret;
 }
 
+static void xt_ct_tg_timeout_put(struct nf_conn *ct)
+{
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+	struct nf_conn_timeout *timeout_ext;
+	typeof(nf_ct_timeout_put_hook) timeout_put;
+
+	rcu_read_lock();
+	timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
+
+	if (timeout_put) {
+		timeout_ext = nf_ct_timeout_find(ct);
+		if (timeout_ext)
+			timeout_put(timeout_ext->timeout);
+	}
+	rcu_read_unlock();
+#endif
+}
+
 static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 {
 	struct xt_ct_target_info_v1 *info = par->targinfo;
@@ -245,7 +263,7 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 				pr_info("Timeout policy `%s' can only be "
 					"used by L3 protocol number %d\n",
 					info->timeout, timeout->l3num);
-				goto err4;
+				goto err5;
 			}
 			/* Make sure the timeout policy matches any existing
 			 * protocol tracker, otherwise default to generic.
@@ -258,13 +276,13 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 					"used by L4 protocol number %d\n",
 					info->timeout,
 					timeout->l4proto->l4proto);
-				goto err4;
+				goto err5;
 			}
 			timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
 							    GFP_ATOMIC);
 			if (timeout_ext == NULL) {
 				ret = -ENOMEM;
-				goto err4;
+				goto err5;
 			}
 		} else {
 			ret = -ENOENT;
@@ -282,6 +300,8 @@ out:
 	return 0;
 
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+err5:
+	xt_ct_tg_timeout_put(ct);
 err4:
 	rcu_read_unlock();
 #endif
@@ -314,28 +334,14 @@ static void xt_ct_tg_destroy_v1(const struct xt_tgdtor_param *par)
 	struct xt_ct_target_info_v1 *info = par->targinfo;
 	struct nf_conn *ct = info->ct;
 	struct nf_conn_help *help;
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
-	struct nf_conn_timeout *timeout_ext;
-	typeof(nf_ct_timeout_put_hook) timeout_put;
-#endif
+
 	if (!nf_ct_is_untracked(ct)) {
 		help = nfct_help(ct);
 		if (help)
 			module_put(help->helper->me);
 
 		nf_ct_l3proto_module_put(par->family);
-
-#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
-		rcu_read_lock();
-		timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
-
-		if (timeout_put) {
-			timeout_ext = nf_ct_timeout_find(ct);
-			if (timeout_ext)
-				timeout_put(timeout_ext->timeout);
-		}
-		rcu_read_unlock();
-#endif
+		xt_ct_tg_timeout_put(ct);
 	}
 	nf_ct_put(info->ct);
 }
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Tetsuo Handa]

Pablo Neira Ayuso wrote:
> Indeed. The patch attached should fix this problem. Thanks.

It seems to me that the timeout object is associated with the "ct"
by successful nf_ct_timeout_ext_add() call. If yes,

 +err5:
 +	xt_ct_tg_timeout_put(ct);

will not be able to find the timeout object.

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 414 bytes --]

On Tue, Apr 03, 2012 at 09:28:38PM +0900, Tetsuo Handa wrote:
> Pablo Neira Ayuso wrote:
> > Indeed. The patch attached should fix this problem. Thanks.
> 
> It seems to me that the timeout object is associated with the "ct"
> by successful nf_ct_timeout_ext_add() call. If yes,
> 
>  +err5:
>  +	xt_ct_tg_timeout_put(ct);
> 
> will not be able to find the timeout object.

You're right again. New patch attached.

[-- Attachment #2: 0001-netfilter-xt_CT-fix-missing-put-timeout-object-in-er.patch --]
[-- Type: text/x-diff, Size: 2728 bytes --]

>From 6b9599e7a092a241e86e376d14bb1b45902f19b5 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 14:50:07 +0200
Subject: [PATCH] netfilter: xt_CT: fix missing put timeout object in error path

The error path misses putting the timeout object. This patch adds
new function xt_ct_tg_timeout_put() to put the timeout object.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 4babb27..e2ee11a 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -150,6 +150,21 @@ err1:
 	return ret;
 }
 
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+static inline void xt_ct_tg_timeout_put(struct ctnl_timeout *timeout)
+{
+	typeof(nf_ct_timeout_put_hook) timeout_put;
+
+	rcu_read_lock();
+	timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
+
+	if (timeout_put)
+		timeout_put(timeout);
+
+	rcu_read_unlock();
+}
+#endif
+
 static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 {
 	struct xt_ct_target_info_v1 *info = par->targinfo;
@@ -158,7 +173,9 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 	struct nf_conn *ct;
 	int ret = 0;
 	u8 proto;
-
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+	struct ctnl_timeout *timeout;
+#endif
 	if (info->flags & ~XT_CT_NOTRACK)
 		return -EINVAL;
 
@@ -216,7 +233,6 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 	if (info->timeout) {
 		typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
-		struct ctnl_timeout *timeout;
 		struct nf_conn_timeout *timeout_ext;
 
 		rcu_read_lock();
@@ -245,7 +261,7 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 				pr_info("Timeout policy `%s' can only be "
 					"used by L3 protocol number %d\n",
 					info->timeout, timeout->l3num);
-				goto err4;
+				goto err5;
 			}
 			/* Make sure the timeout policy matches any existing
 			 * protocol tracker, otherwise default to generic.
@@ -258,13 +274,13 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 					"used by L4 protocol number %d\n",
 					info->timeout,
 					timeout->l4proto->l4proto);
-				goto err4;
+				goto err5;
 			}
 			timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
 							    GFP_ATOMIC);
 			if (timeout_ext == NULL) {
 				ret = -ENOMEM;
-				goto err4;
+				goto err5;
 			}
 		} else {
 			ret = -ENOENT;
@@ -282,6 +298,8 @@ out:
 	return 0;
 
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+err5:
+	xt_ct_tg_timeout_put(timeout);
 err4:
 	rcu_read_unlock();
 #endif
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Tue, Apr 03, 2012 at 02:57:55PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Apr 03, 2012 at 09:28:38PM +0900, Tetsuo Handa wrote:
> > Pablo Neira Ayuso wrote:
> > > Indeed. The patch attached should fix this problem. Thanks.
> > 
> > It seems to me that the timeout object is associated with the "ct"
> > by successful nf_ct_timeout_ext_add() call. If yes,
> > 
> >  +err5:
> >  +	xt_ct_tg_timeout_put(ct);
> > 
> > will not be able to find the timeout object.
> 
> You're right again. New patch attached.

> From 6b9599e7a092a241e86e376d14bb1b45902f19b5 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 3 Apr 2012 14:50:07 +0200
> Subject: [PATCH] netfilter: xt_CT: fix missing put timeout object in error path

A bit better patch, no need to hold rcu_read_lock again in
xt_ct_tg_timeout_put (even if that doesn't harm).

[-- Attachment #2: 0001-netfilter-xt_CT-fix-missing-put-timeout-object-in-er.patch --]
[-- Type: text/x-diff, Size: 2677 bytes --]

>From 96a1ee814eef63a917e2a0af7a70bbf85511d285 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 14:50:07 +0200
Subject: [PATCH] netfilter: xt_CT: fix missing put timeout object in error path

The error path misses putting the timeout object. This patch adds
new function xt_ct_tg_timeout_put() to put the timeout object.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/xt_CT.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 4babb27..59530e9 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -150,6 +150,17 @@ err1:
 	return ret;
 }
 
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+static void __xt_ct_tg_timeout_put(struct ctnl_timeout *timeout)
+{
+	typeof(nf_ct_timeout_put_hook) timeout_put;
+
+	timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
+	if (timeout_put)
+		timeout_put(timeout);
+}
+#endif
+
 static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 {
 	struct xt_ct_target_info_v1 *info = par->targinfo;
@@ -158,7 +169,9 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 	struct nf_conn *ct;
 	int ret = 0;
 	u8 proto;
-
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+	struct ctnl_timeout *timeout;
+#endif
 	if (info->flags & ~XT_CT_NOTRACK)
 		return -EINVAL;
 
@@ -216,7 +229,6 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 	if (info->timeout) {
 		typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
-		struct ctnl_timeout *timeout;
 		struct nf_conn_timeout *timeout_ext;
 
 		rcu_read_lock();
@@ -245,7 +257,7 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 				pr_info("Timeout policy `%s' can only be "
 					"used by L3 protocol number %d\n",
 					info->timeout, timeout->l3num);
-				goto err4;
+				goto err5;
 			}
 			/* Make sure the timeout policy matches any existing
 			 * protocol tracker, otherwise default to generic.
@@ -258,13 +270,13 @@ static int xt_ct_tg_check_v1(const struct xt_tgchk_param *par)
 					"used by L4 protocol number %d\n",
 					info->timeout,
 					timeout->l4proto->l4proto);
-				goto err4;
+				goto err5;
 			}
 			timeout_ext = nf_ct_timeout_ext_add(ct, timeout,
 							    GFP_ATOMIC);
 			if (timeout_ext == NULL) {
 				ret = -ENOMEM;
-				goto err4;
+				goto err5;
 			}
 		} else {
 			ret = -ENOENT;
@@ -282,6 +294,8 @@ out:
 	return 0;
 
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+err5:
+	__xt_ct_tg_timeout_put(timeout);
 err4:
 	rcu_read_unlock();
 #endif
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Tetsuo Handa]

Pablo Neira Ayuso wrote:
> A bit better patch, no need to hold rcu_read_lock again in
> xt_ct_tg_timeout_put (even if that doesn't harm).

Patch looks OK.

By the way,

665 static struct nf_conn *
666 __nf_conntrack_alloc(struct net *net, u16 zone,
667                      const struct nf_conntrack_tuple *orig,
668                      const struct nf_conntrack_tuple *repl,
669                      gfp_t gfp, u32 hash)
670 {
(...snipped...)
680         atomic_inc(&net->ct.count);
(...snipped...)
698         ct = kmem_cache_alloc(net->ct.nf_conntrack_cachep, gfp);
699         if (ct == NULL) {
700                 atomic_dec(&net->ct.count);
701                 return ERR_PTR(-ENOMEM);
702         }
(...snipped...)
720         if (zone) {
721                 struct nf_conntrack_zone *nf_ct_zone;
722 
723                 nf_ct_zone = nf_ct_ext_add(ct, NF_CT_EXT_ZONE, GFP_ATOMIC);
724                 if (!nf_ct_zone)
725                         goto out_free;
726                 nf_ct_zone->id = zone;
727         }
(...snipped...)
737 out_free:

I think we want to call atomic_dec() here.

738         kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
739         return ERR_PTR(-ENOMEM);
740 #endif
741 }

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Tue, Apr 03, 2012 at 11:28:50PM +0900, Tetsuo Handa wrote:
> Pablo Neira Ayuso wrote:
> > A bit better patch, no need to hold rcu_read_lock again in
> > xt_ct_tg_timeout_put (even if that doesn't harm).
> 
> Patch looks OK.
> 
> By the way,
> 
> 665 static struct nf_conn *
> 666 __nf_conntrack_alloc(struct net *net, u16 zone,
> 667                      const struct nf_conntrack_tuple *orig,
> 668                      const struct nf_conntrack_tuple *repl,
> 669                      gfp_t gfp, u32 hash)
> 670 {
> (...snipped...)
> 680         atomic_inc(&net->ct.count);
> (...snipped...)
> 698         ct = kmem_cache_alloc(net->ct.nf_conntrack_cachep, gfp);
> 699         if (ct == NULL) {
> 700                 atomic_dec(&net->ct.count);
> 701                 return ERR_PTR(-ENOMEM);
> 702         }
> (...snipped...)
> 720         if (zone) {
> 721                 struct nf_conntrack_zone *nf_ct_zone;
> 722 
> 723                 nf_ct_zone = nf_ct_ext_add(ct, NF_CT_EXT_ZONE, GFP_ATOMIC);
> 724                 if (!nf_ct_zone)
> 725                         goto out_free;
> 726                 nf_ct_zone->id = zone;
> 727         }
> (...snipped...)
> 737 out_free:
> 
> I think we want to call atomic_dec() here.

Good catch :-).

Patch to address this issue follows.

[-- Attachment #2: 0001-netfilter-nf_conntrack-fix-count-leak-in-error-path-.patch --]
[-- Type: text/x-diff, Size: 991 bytes --]

>From 0fca4d604c0898128014bc03ef965e0cf187a5e9 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 16:45:54 +0200
Subject: [PATCH] netfilter: nf_conntrack: fix count leak in error path of __nf_conntrack_alloc

We have to decrement the conntrack counter if we fail to access the
zone extension.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index cbdb754..3cc4487 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -735,6 +735,7 @@ __nf_conntrack_alloc(struct net *net, u16 zone,
 
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 out_free:
+	atomic_dec(&net->ct.count);
 	kmem_cache_free(net->ct.nf_conntrack_cachep, ct);
 	return ERR_PTR(-ENOMEM);
 #endif
-- 
1.7.2.5

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 12:39:03 +0200

>>From e21e9622a46a60bd2b8dfb9791bb63dfdd4940d7 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 3 Apr 2012 12:32:15 +0200
> Subject: [PATCH] netfilter: xt_CT: allocation has to be GFP_ATOMIC under rcu_read_lock section
> 
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied.

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 15:06:20 +0200

>>From 96a1ee814eef63a917e2a0af7a70bbf85511d285 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 3 Apr 2012 14:50:07 +0200
> Subject: [PATCH] netfilter: xt_CT: fix missing put timeout object in error path
> 
> The error path misses putting the timeout object. This patch adds
> new function xt_ct_tg_timeout_put() to put the timeout object.
> 
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied.

Re: [PATCH 3/3] netfilter: xt_CT: remove a compile warning

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 3 Apr 2012 16:52:10 +0200

>>From 0fca4d604c0898128014bc03ef965e0cf187a5e9 Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 3 Apr 2012 16:45:54 +0200
> Subject: [PATCH] netfilter: nf_conntrack: fix count leak in error path of __nf_conntrack_alloc
> 
> We have to decrement the conntrack counter if we fail to access the
> zone extension.
> 
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied.