[PATCH 4/7] Helper modules load on-demand support for ctnetlink

[PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

[PATCH] Helper modules load on-demand support for ctnetlink

This patch adds helper load on-demand support for ctnetlink. This patch
also refactorizes the code to provide a couple of functions to look up
and set the connection tracking helper. The function removes the
exported symbol __nf_ct_helper_find as it has not clients anymore.

This patch uses the persistent helper aliasing defined in the previous
patch.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

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

[-- Attachment #2: 03.patch --]
[-- Type: text/x-diff, Size: 6668 bytes --]

[PATCH] Helper modules load on-demand support for ctnetlink

This patch adds helper load on-demand support for ctnetlink. This patch
also refactorizes the code to provide a couple of functions to look up 
and set the connection tracking helper. The function removes the exported
symbol __nf_ct_helper_find as it has not clients anymore.

This patch uses the persistent helper aliasing defined in the previous 
patch.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_core.c	2008-07-30 08:50:04.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_core.c	2008-07-30 08:54:09.000000000 +0200
@@ -578,14 +578,7 @@ init_conntrack(const struct nf_conntrack
 		nf_conntrack_get(&ct->master->ct_general);
 		NF_CT_STAT_INC(expect_new);
 	} else {
-		struct nf_conntrack_helper *helper;
-
-		helper = __nf_ct_helper_find(&repl_tuple);
-		if (helper) {
-			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-			if (help)
-				rcu_assign_pointer(help->helper, helper);
-		}
+		__nf_ct_set_helper(ct, GFP_ATOMIC);
 		NF_CT_STAT_INC(new);
 	}
 
@@ -757,7 +750,6 @@ void nf_conntrack_alter_reply(struct nf_
 			      const struct nf_conntrack_tuple *newreply)
 {
 	struct nf_conn_help *help = nfct_help(ct);
-	struct nf_conntrack_helper *helper;
 
 	/* Should be unconfirmed, so not in hash table yet */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
@@ -769,25 +761,7 @@ void nf_conntrack_alter_reply(struct nf_
 	if (ct->master || (help && !hlist_empty(&help->expectations)))
 		return;
 
-	rcu_read_lock();
-	helper = __nf_ct_helper_find(newreply);
-	if (helper == NULL) {
-		if (help)
-			rcu_assign_pointer(help->helper, NULL);
-		goto out;
-	}
-
-	if (help == NULL) {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			goto out;
-	} else {
-		memset(&help->help, 0, sizeof(help->help));
-	}
-
-	rcu_assign_pointer(help->helper, helper);
-out:
-	rcu_read_unlock();
+	nf_ct_set_helper(ct, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
 
Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-30 08:50:04.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-30 09:00:05.000000000 +0200
@@ -1120,8 +1120,6 @@ ctnetlink_create_conntrack(struct nlattr
 {
 	struct nf_conn *ct;
 	int err = -EINVAL;
-	struct nf_conn_help *help;
-	struct nf_conntrack_helper *helper;
 
 	ct = nf_conntrack_alloc(otuple, rtuple, GFP_KERNEL);
 	if (ct == NULL || IS_ERR(ct))
@@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
 	rcu_read_lock();
-	helper = __nf_ct_helper_find(rtuple);
-	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
-		if (help == NULL) {
-			rcu_read_unlock();
-			err = -ENOMEM;
-			goto err;
-		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
-	}
+	__nf_ct_set_helper(ct, GFP_KERNEL);
 
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
@@ -1672,9 +1660,24 @@ ctnetlink_create_expect(struct nlattr *c
 	help = nfct_help(ct);
 
 	if (!help || !help->helper) {
-		/* such conntrack hasn't got any helper, abort */
+#ifdef CONFIG_KMOD
+		char *name;
+
 		err = -EINVAL;
+		if (!cda[CTA_EXPECT_HELP_NAME])
+			goto out;
+
+		err = -ENOTSUPP;
+		name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
+		if (request_module("nfct-helper-%s", name) < 0)
+			goto out;
+
+		if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
+			goto out;
+#else
+		err = -ENOTSUPP;
 		goto out;
+#endif
 	}
 
 	exp = nf_ct_expect_alloc(ct);
Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
--- net-next-2.6.git.orig/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 08:50:04.000000000 +0200
+++ net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 08:54:09.000000000 +0200
@@ -39,9 +39,6 @@ struct nf_conntrack_helper
 };
 
 extern struct nf_conntrack_helper *
-__nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
-
-extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);
@@ -49,6 +46,19 @@ extern void nf_conntrack_helper_unregist
 
 extern struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 
+extern int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags);
+
+static inline int nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = __nf_ct_set_helper(ct, flags);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_helper.c	2008-07-30 08:50:04.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_helper.c	2008-07-30 08:54:09.000000000 +0200
@@ -43,7 +43,7 @@ static unsigned int helper_hash(const st
 		(__force __u16)tuple->src.u.all) % nf_ct_helper_hsize;
 }
 
-struct nf_conntrack_helper *
+static struct nf_conntrack_helper *
 __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
@@ -61,7 +61,6 @@ __nf_ct_helper_find(const struct nf_conn
 	}
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(__nf_ct_helper_find);
 
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name)
@@ -93,6 +92,36 @@ struct nf_conn_help *nf_ct_helper_ext_ad
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_set_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> [PATCH] Helper modules load on-demand support for ctnetlink
>
> This patch adds helper load on-demand support for ctnetlink. This patch
> also refactorizes the code to provide a couple of functions to look up
> and set the connection tracking helper. The function removes the
> exported symbol __nf_ct_helper_find as it has not clients anymore.
>
> This patch uses the persistent helper aliasing defined in the previous
> patch.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
>   
> Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
> ===================================================================
> --- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-30 08:50:04.000000000 +0200
> +++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-30 09:00:05.000000000 +0200
> @@ -1120,8 +1120,6 @@ ctnetlink_create_conntrack(struct nlattr
>  {
>  	struct nf_conn *ct;
>  	int err = -EINVAL;
> -	struct nf_conn_help *help;
> -	struct nf_conntrack_helper *helper;
>  
>  	ct = nf_conntrack_alloc(otuple, rtuple, GFP_KERNEL);
>  	if (ct == NULL || IS_ERR(ct))
> @@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
>  #endif
>  
>  	rcu_read_lock();
> -	helper = __nf_ct_helper_find(rtuple);
> -	if (helper) {
> -		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
> -		if (help == NULL) {
> -			rcu_read_unlock();
> -			err = -ENOMEM;
> -			goto err;
> -		}
> -		/* not in hash table yet so not strictly necessary */
> -		rcu_assign_pointer(help->helper, helper);
> -	}
> +	__nf_ct_set_helper(ct, GFP_KERNEL);

This one should be checking the return value.

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
>> @@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
>>  #endif
>>  
>>      rcu_read_lock();
>> -    helper = __nf_ct_helper_find(rtuple);
>> -    if (helper) {
>> -        help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
>> -        if (help == NULL) {
>> -            rcu_read_unlock();
>> -            err = -ENOMEM;
>> -            goto err;
>> -        }
>> -        /* not in hash table yet so not strictly necessary */
>> -        rcu_assign_pointer(help->helper, helper);
>> -    }
>> +    __nf_ct_set_helper(ct, GFP_KERNEL);
> 
> This one should be checking the return value.

Attached a new version.

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

[-- Attachment #2: 03.patch --]
[-- Type: text/x-diff, Size: 6703 bytes --]

[PATCHv2] Helper modules load on-demand support for ctnetlink

This patch adds helper load on-demand support for ctnetlink. This patch
also refactorizes the code to provide a couple of functions to look up 
and set the connection tracking helper. The function removes the exported
symbol __nf_ct_helper_find as it has not clients anymore.

This patch uses the persistent helper aliasing defined in the previous 
patch.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_core.c	2008-07-30 13:19:39.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_core.c	2008-07-30 13:19:42.000000000 +0200
@@ -578,14 +578,7 @@ init_conntrack(const struct nf_conntrack
 		nf_conntrack_get(&ct->master->ct_general);
 		NF_CT_STAT_INC(expect_new);
 	} else {
-		struct nf_conntrack_helper *helper;
-
-		helper = __nf_ct_helper_find(&repl_tuple);
-		if (helper) {
-			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-			if (help)
-				rcu_assign_pointer(help->helper, helper);
-		}
+		__nf_ct_set_helper(ct, GFP_ATOMIC);
 		NF_CT_STAT_INC(new);
 	}
 
@@ -757,7 +750,6 @@ void nf_conntrack_alter_reply(struct nf_
 			      const struct nf_conntrack_tuple *newreply)
 {
 	struct nf_conn_help *help = nfct_help(ct);
-	struct nf_conntrack_helper *helper;
 
 	/* Should be unconfirmed, so not in hash table yet */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
@@ -769,25 +761,7 @@ void nf_conntrack_alter_reply(struct nf_
 	if (ct->master || (help && !hlist_empty(&help->expectations)))
 		return;
 
-	rcu_read_lock();
-	helper = __nf_ct_helper_find(newreply);
-	if (helper == NULL) {
-		if (help)
-			rcu_assign_pointer(help->helper, NULL);
-		goto out;
-	}
-
-	if (help == NULL) {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			goto out;
-	} else {
-		memset(&help->help, 0, sizeof(help->help));
-	}
-
-	rcu_assign_pointer(help->helper, helper);
-out:
-	rcu_read_unlock();
+	nf_ct_set_helper(ct, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
 
Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-30 13:19:39.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-30 13:25:07.000000000 +0200
@@ -1120,8 +1120,6 @@ ctnetlink_create_conntrack(struct nlattr
 {
 	struct nf_conn *ct;
 	int err = -EINVAL;
-	struct nf_conn_help *help;
-	struct nf_conntrack_helper *helper;
 
 	ct = nf_conntrack_alloc(otuple, rtuple, GFP_KERNEL);
 	if (ct == NULL || IS_ERR(ct))
@@ -1152,16 +1150,9 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
 	rcu_read_lock();
-	helper = __nf_ct_helper_find(rtuple);
-	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
-		if (help == NULL) {
-			rcu_read_unlock();
-			err = -ENOMEM;
-			goto err;
-		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
+	if (__nf_ct_set_helper(ct, GFP_KERNEL) == -ENOMEM) {
+		rcu_read_unlock();
+		goto err;
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */
@@ -1672,9 +1663,24 @@ ctnetlink_create_expect(struct nlattr *c
 	help = nfct_help(ct);
 
 	if (!help || !help->helper) {
-		/* such conntrack hasn't got any helper, abort */
+#ifdef CONFIG_KMOD
+		char *name;
+
 		err = -EINVAL;
+		if (!cda[CTA_EXPECT_HELP_NAME])
+			goto out;
+
+		err = -ENOTSUPP;
+		name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
+		if (request_module("nfct-helper-%s", name) < 0)
+			goto out;
+
+		if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
+			goto out;
+#else
+		err = -ENOTSUPP;
 		goto out;
+#endif
 	}
 
 	exp = nf_ct_expect_alloc(ct);
Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
--- net-next-2.6.git.orig/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 13:19:39.000000000 +0200
+++ net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 13:19:42.000000000 +0200
@@ -39,9 +39,6 @@ struct nf_conntrack_helper
 };
 
 extern struct nf_conntrack_helper *
-__nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
-
-extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);
@@ -49,6 +46,19 @@ extern void nf_conntrack_helper_unregist
 
 extern struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 
+extern int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags);
+
+static inline int nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = __nf_ct_set_helper(ct, flags);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_helper.c	2008-07-30 13:19:39.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_helper.c	2008-07-30 13:19:42.000000000 +0200
@@ -43,7 +43,7 @@ static unsigned int helper_hash(const st
 		(__force __u16)tuple->src.u.all) % nf_ct_helper_hsize;
 }
 
-struct nf_conntrack_helper *
+static struct nf_conntrack_helper *
 __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
@@ -61,7 +61,6 @@ __nf_ct_helper_find(const struct nf_conn
 	}
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(__nf_ct_helper_find);
 
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name)
@@ -93,6 +92,36 @@ struct nf_conn_help *nf_ct_helper_ext_ad
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_set_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>>> @@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
>>>  #endif
>>>  
>>>      rcu_read_lock();
>>> -    helper = __nf_ct_helper_find(rtuple);
>>> -    if (helper) {
>>> -        help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
>>> -        if (help == NULL) {
>>> -            rcu_read_unlock();
>>> -            err = -ENOMEM;
>>> -            goto err;
>>> -        }
>>> -        /* not in hash table yet so not strictly necessary */
>>> -        rcu_assign_pointer(help->helper, helper);
>>> -    }
>>> +    __nf_ct_set_helper(ct, GFP_KERNEL);
>> This one should be checking the return value.
> 
> Attached a new version.

Sorry, it's wrong. Please, take this.

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

[-- Attachment #2: 03.patch --]
[-- Type: text/x-diff, Size: 6722 bytes --]

[PATCHv3] Helper modules load on-demand support for ctnetlink

This patch adds helper load on-demand support for ctnetlink. This patch
also refactorizes the code to provide a couple of functions to look up 
and set the connection tracking helper. The function removes the exported
symbol __nf_ct_helper_find as it has not clients anymore.

This patch uses the persistent helper aliasing defined in the previous 
patch.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_core.c	2008-07-30 13:32:31.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_core.c	2008-07-30 13:32:34.000000000 +0200
@@ -578,14 +578,7 @@ init_conntrack(const struct nf_conntrack
 		nf_conntrack_get(&ct->master->ct_general);
 		NF_CT_STAT_INC(expect_new);
 	} else {
-		struct nf_conntrack_helper *helper;
-
-		helper = __nf_ct_helper_find(&repl_tuple);
-		if (helper) {
-			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-			if (help)
-				rcu_assign_pointer(help->helper, helper);
-		}
+		__nf_ct_set_helper(ct, GFP_ATOMIC);
 		NF_CT_STAT_INC(new);
 	}
 
@@ -757,7 +750,6 @@ void nf_conntrack_alter_reply(struct nf_
 			      const struct nf_conntrack_tuple *newreply)
 {
 	struct nf_conn_help *help = nfct_help(ct);
-	struct nf_conntrack_helper *helper;
 
 	/* Should be unconfirmed, so not in hash table yet */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
@@ -769,25 +761,7 @@ void nf_conntrack_alter_reply(struct nf_
 	if (ct->master || (help && !hlist_empty(&help->expectations)))
 		return;
 
-	rcu_read_lock();
-	helper = __nf_ct_helper_find(newreply);
-	if (helper == NULL) {
-		if (help)
-			rcu_assign_pointer(help->helper, NULL);
-		goto out;
-	}
-
-	if (help == NULL) {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			goto out;
-	} else {
-		memset(&help->help, 0, sizeof(help->help));
-	}
-
-	rcu_assign_pointer(help->helper, helper);
-out:
-	rcu_read_unlock();
+	nf_ct_set_helper(ct, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
 
Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-30 13:32:31.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-30 13:33:13.000000000 +0200
@@ -1120,8 +1120,6 @@ ctnetlink_create_conntrack(struct nlattr
 {
 	struct nf_conn *ct;
 	int err = -EINVAL;
-	struct nf_conn_help *help;
-	struct nf_conntrack_helper *helper;
 
 	ct = nf_conntrack_alloc(otuple, rtuple, GFP_KERNEL);
 	if (ct == NULL || IS_ERR(ct))
@@ -1152,16 +1150,10 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
 	rcu_read_lock();
-	helper = __nf_ct_helper_find(rtuple);
-	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
-		if (help == NULL) {
-			rcu_read_unlock();
-			err = -ENOMEM;
-			goto err;
-		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
+	if (__nf_ct_set_helper(ct, GFP_KERNEL) == -ENOMEM) {
+		rcu_read_unlock();
+		err = -ENOMEM;
+		goto err;
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */
@@ -1672,9 +1664,24 @@ ctnetlink_create_expect(struct nlattr *c
 	help = nfct_help(ct);
 
 	if (!help || !help->helper) {
-		/* such conntrack hasn't got any helper, abort */
+#ifdef CONFIG_KMOD
+		char *name;
+
 		err = -EINVAL;
+		if (!cda[CTA_EXPECT_HELP_NAME])
+			goto out;
+
+		err = -ENOTSUPP;
+		name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
+		if (request_module("nfct-helper-%s", name) < 0)
+			goto out;
+
+		if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
+			goto out;
+#else
+		err = -ENOTSUPP;
 		goto out;
+#endif
 	}
 
 	exp = nf_ct_expect_alloc(ct);
Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
--- net-next-2.6.git.orig/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 13:32:31.000000000 +0200
+++ net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h	2008-07-30 13:32:34.000000000 +0200
@@ -39,9 +39,6 @@ struct nf_conntrack_helper
 };
 
 extern struct nf_conntrack_helper *
-__nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
-
-extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);
@@ -49,6 +46,19 @@ extern void nf_conntrack_helper_unregist
 
 extern struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 
+extern int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags);
+
+static inline int nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = __nf_ct_set_helper(ct, flags);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_helper.c	2008-07-30 13:32:31.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_helper.c	2008-07-30 13:32:34.000000000 +0200
@@ -43,7 +43,7 @@ static unsigned int helper_hash(const st
 		(__force __u16)tuple->src.u.all) % nf_ct_helper_hsize;
 }
 
-struct nf_conntrack_helper *
+static struct nf_conntrack_helper *
 __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
@@ -61,7 +61,6 @@ __nf_ct_helper_find(const struct nf_conn
 	}
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(__nf_ct_helper_find);
 
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name)
@@ -93,6 +92,36 @@ struct nf_conn_help *nf_ct_helper_ext_ad
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_set_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Sorry, it's wrong. Please, take this.

> @@ -1672,9 +1660,24 @@ ctnetlink_create_expect(struct nlattr *c
>  	help = nfct_help(ct);
>  
>  	if (!help || !help->helper) {
> -		/* such conntrack hasn't got any helper, abort */
> +#ifdef CONFIG_KMOD
> +		char *name;
> +
>  		err = -EINVAL;
> +		if (!cda[CTA_EXPECT_HELP_NAME])
> +			goto out;
> +
> +		err = -ENOTSUPP;
> +		name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
> +		if (request_module("nfct-helper-%s", name) < 0)
> +			goto out;
> +
> +		if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
> +			goto out;

This strikes me as quite inconsistent. First, we only perform
autoloading for expectation creation, but not for conntracks.
Second, this implicit helper assignment is also a bit unusual,
why don't we simply insist that the conntrack has a helper
assigned through the ctnetlink conntrack interface?

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Sorry, it's wrong. Please, take this.
> 
>> @@ -1672,9 +1660,24 @@ ctnetlink_create_expect(struct nlattr *c
>>      help = nfct_help(ct);
>>  
>>      if (!help || !help->helper) {
>> -        /* such conntrack hasn't got any helper, abort */
>> +#ifdef CONFIG_KMOD
>> +        char *name;
>> +
>>          err = -EINVAL;
>> +        if (!cda[CTA_EXPECT_HELP_NAME])
>> +            goto out;
>> +
>> +        err = -ENOTSUPP;
>> +        name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
>> +        if (request_module("nfct-helper-%s", name) < 0)
>> +            goto out;
>> +
>> +        if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
>> +            goto out;
> 
> This strikes me as quite inconsistent. First, we only perform
> autoloading for expectation creation, but not for conntracks.

The module autoloading for conntracks is tricky, it's easy to add for
the creation case, but I don't see a sane way to do this in the update
case because of the spin lock that we hold most of the time.

The only idea that comes to my mind is to do the module load-on-demand
in a very early stage - just after the tuple parsing in the
new_conntrack function - but then we'll have to do another look up for
the helper inside the change_helper function - that would make two
invocations of find_byname() to assign the helper.

Moreover, someone may remove the module in the middle just after the
module loading but, well, we have lost the race in the case.

> Second, this implicit helper assignment is also a bit unusual,
> why don't we simply insist that the conntrack has a helper
> assigned through the ctnetlink conntrack interface?

If I understood well, then we simply assign the helper to the conntrack
and the expectation part of ctnetlink should rely on the existing
assigned helper, right?

Please, have a look at the patch attached.

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

[-- Attachment #2: 03.patch --]
[-- Type: text/x-diff, Size: 1505 bytes --]

[PATCH] Helper modules load-on-demand support for ctnetlink

This patch adds module loading for helpers via ctnetlink. We perform
the module loading before we enter the spin lock area. Thus, the number
of lookups to assign a helper is two: one to check if the helper is
present and one to assign it.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-31 10:15:37.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-31 10:33:13.000000000 +0200
@@ -1203,6 +1203,35 @@ ctnetlink_new_conntrack(struct sock *ctn
 			return err;
 	}
 
+	if (cda[CTA_HELP]) {
+		const struct nf_conntrack_helper *helper;
+		char *helpname;
+
+		err = ctnetlink_parse_help(cda[CTA_HELP], &helpname);
+		if (err < 0)
+			return err;
+
+		rcu_read_lock();
+		helper = __nf_conntrack_helper_find_byname(helpname);
+		if (helper == NULL) {
+			rcu_read_unlock();
+#ifdef CONFIG_KMOD
+			if (request_module("nfct-helper-%s", helpname) < 0)
+				return -EOPNOTSUPP;
+
+			rcu_read_lock();
+			helper = __nf_conntrack_helper_find_byname(helpname);
+			if (helper == NULL) {
+				rcu_read_unlock();
+				return -EOPNOTSUPP;
+			}
+#else
+			return -EOPNOTSUPP;
+#endif
+		}
+		rcu_read_unlock();
+	}
+
 	spin_lock_bh(&nf_conntrack_lock);
 	if (cda[CTA_TUPLE_ORIG])
 		h = __nf_conntrack_find(&otuple);

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Sorry, it's wrong. Please, take this.
>>> @@ -1672,9 +1660,24 @@ ctnetlink_create_expect(struct nlattr *c
>>>      help = nfct_help(ct);
>>>  
>>>      if (!help || !help->helper) {
>>> -        /* such conntrack hasn't got any helper, abort */
>>> +#ifdef CONFIG_KMOD
>>> +        char *name;
>>> +
>>>          err = -EINVAL;
>>> +        if (!cda[CTA_EXPECT_HELP_NAME])
>>> +            goto out;
>>> +
>>> +        err = -ENOTSUPP;
>>> +        name = nla_data(cda[CTA_EXPECT_HELP_NAME]);
>>> +        if (request_module("nfct-helper-%s", name) < 0)
>>> +            goto out;
>>> +
>>> +        if (nf_ct_set_helper(ct, GFP_KERNEL) < 0)
>>> +            goto out;
>> This strikes me as quite inconsistent. First, we only perform
>> autoloading for expectation creation, but not for conntracks.
> 
> The module autoloading for conntracks is tricky, it's easy to add for
> the creation case, but I don't see a sane way to do this in the update
> case because of the spin lock that we hold most of the time.
> 
> The only idea that comes to my mind is to do the module load-on-demand
> in a very early stage - just after the tuple parsing in the
> new_conntrack function - but then we'll have to do another look up for
> the helper inside the change_helper function - that would make two
> invocations of find_byname() to assign the helper.
> 
> Moreover, someone may remove the module in the middle just after the
> module loading but, well, we have lost the race in the case.

I'd do something similar to qdiscs etc:

- lookup helper
- if not found: request_module, take lock again, repeat lookup, return 
EAGAIN if found now
- in the nfnetlink command handler: if ret == EAGAIN replay message

grep for "replay" in net/ for a few examples of this. This also
handles the race BTW.

>> Second, this implicit helper assignment is also a bit unusual,
>> why don't we simply insist that the conntrack has a helper
>> assigned through the ctnetlink conntrack interface?
> 
> If I understood well, then we simply assign the helper to the conntrack
> and the expectation part of ctnetlink should rely on the existing
> assigned helper, right?

Yes, I think thats cleaner.

> 
> Please, have a look at the patch attached.

Looks fine.

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Moreover, someone may remove the module in the middle just after the
>> module loading but, well, we have lost the race in the case.
> 
> I'd do something similar to qdiscs etc:
> 
> - lookup helper
> - if not found: request_module, take lock again, repeat lookup, return
> EAGAIN if found now
> - in the nfnetlink command handler: if ret == EAGAIN replay message
> 
> grep for "replay" in net/ for a few examples of this. This also
> handles the race BTW.

Nice. Thanks for the explanation :). I'll check if I can cook a couple
of patches to do it that way at some point.

>>> Second, this implicit helper assignment is also a bit unusual,
>>> why don't we simply insist that the conntrack has a helper
>>> assigned through the ctnetlink conntrack interface?
>>
>> If I understood well, then we simply assign the helper to the conntrack
>> and the expectation part of ctnetlink should rely on the existing
>> assigned helper, right?
> 
> Yes, I think thats cleaner.
>>
>> Please, have a look at the patch attached.
> 
> Looks fine.

Would you also apply the following two patches before looking at [PATCH
5/7]?

The first patch is a refactorization that was in the previous version of
4/7, the latter is a minor return value change to make it consistent
with others error values in ctnetlink.

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

[-- Attachment #2: 04.patch --]
[-- Type: text/x-diff, Size: 5975 bytes --]

[PATCH] helper assignation code refactorization

This refactorization provides a function to lookup-and-set the
connection tracking helper. The function removes the exported symbol
__nf_ct_helper_find as it has not clients anymore.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_core.c	2008-07-31 09:14:25.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_core.c	2008-07-31 11:07:55.000000000 +0200
@@ -578,14 +578,7 @@ init_conntrack(const struct nf_conntrack
 		nf_conntrack_get(&ct->master->ct_general);
 		NF_CT_STAT_INC(expect_new);
 	} else {
-		struct nf_conntrack_helper *helper;
-
-		helper = __nf_ct_helper_find(&repl_tuple);
-		if (helper) {
-			help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-			if (help)
-				rcu_assign_pointer(help->helper, helper);
-		}
+		__nf_ct_set_helper(ct, GFP_ATOMIC);
 		NF_CT_STAT_INC(new);
 	}
 
@@ -757,7 +750,6 @@ void nf_conntrack_alter_reply(struct nf_
 			      const struct nf_conntrack_tuple *newreply)
 {
 	struct nf_conn_help *help = nfct_help(ct);
-	struct nf_conntrack_helper *helper;
 
 	/* Should be unconfirmed, so not in hash table yet */
 	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
@@ -769,25 +761,7 @@ void nf_conntrack_alter_reply(struct nf_
 	if (ct->master || (help && !hlist_empty(&help->expectations)))
 		return;
 
-	rcu_read_lock();
-	helper = __nf_ct_helper_find(newreply);
-	if (helper == NULL) {
-		if (help)
-			rcu_assign_pointer(help->helper, NULL);
-		goto out;
-	}
-
-	if (help == NULL) {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			goto out;
-	} else {
-		memset(&help->help, 0, sizeof(help->help));
-	}
-
-	rcu_assign_pointer(help->helper, helper);
-out:
-	rcu_read_unlock();
+	nf_ct_set_helper(ct, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_alter_reply);
 
Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
--- net-next-2.6.git.orig/include/net/netfilter/nf_conntrack_helper.h	2008-07-31 10:31:08.000000000 +0200
+++ net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h	2008-07-31 11:07:55.000000000 +0200
@@ -39,9 +39,6 @@ struct nf_conntrack_helper
 };
 
 extern struct nf_conntrack_helper *
-__nf_ct_helper_find(const struct nf_conntrack_tuple *tuple);
-
-extern struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name);
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);
@@ -49,6 +46,19 @@ extern void nf_conntrack_helper_unregist
 
 extern struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
 
+extern int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags);
+
+static inline int nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = __nf_ct_set_helper(ct, flags);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_HELPER);
Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_helper.c	2008-07-31 09:14:25.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_helper.c	2008-07-31 11:07:55.000000000 +0200
@@ -43,7 +43,7 @@ static unsigned int helper_hash(const st
 		(__force __u16)tuple->src.u.all) % nf_ct_helper_hsize;
 }
 
-struct nf_conntrack_helper *
+static struct nf_conntrack_helper *
 __nf_ct_helper_find(const struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
@@ -61,7 +61,6 @@ __nf_ct_helper_find(const struct nf_conn
 	}
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(__nf_ct_helper_find);
 
 struct nf_conntrack_helper *
 __nf_conntrack_helper_find_byname(const char *name)
@@ -93,6 +92,36 @@ struct nf_conn_help *nf_ct_helper_ext_ad
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+int __nf_ct_set_helper(struct nf_conn *ct, gfp_t flags)
+{
+	int ret = 0;
+	struct nf_conntrack_helper *helper;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	if (helper == NULL) {
+		if (help)
+			rcu_assign_pointer(help->helper, NULL);
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (help == NULL) {
+		help = nf_ct_helper_ext_add(ct, flags);
+		if (help == NULL) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	} else {
+		memset(&help->help, 0, sizeof(help->help));
+	}
+
+	rcu_assign_pointer(help->helper, helper);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__nf_ct_set_helper);
+
 static inline int unhelp(struct nf_conntrack_tuple_hash *i,
 			 const struct nf_conntrack_helper *me)
 {
Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-31 11:32:17.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-31 11:40:32.000000000 +0200
@@ -1120,8 +1120,6 @@ ctnetlink_create_conntrack(struct nlattr
 {
 	struct nf_conn *ct;
 	int err = -EINVAL;
-	struct nf_conn_help *help;
-	struct nf_conntrack_helper *helper;
 
 	ct = nf_conntrack_alloc(otuple, rtuple, GFP_KERNEL);
 	if (ct == NULL || IS_ERR(ct))
@@ -1152,16 +1150,10 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
 	rcu_read_lock();
-	helper = __nf_ct_helper_find(rtuple);
-	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
-		if (help == NULL) {
-			rcu_read_unlock();
-			err = -ENOMEM;
-			goto err;
-		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
+	if (__nf_ct_set_helper(ct, GFP_KERNEL) == -ENOMEM) {
+		rcu_read_unlock();
+		err = -ENOMEM;
+		goto err;
 	}
 
 	/* setup master conntrack: this is a confirmed expectation */

[-- Attachment #3: 04b.patch --]
[-- Type: text/x-diff, Size: 823 bytes --]

[PATCH] use EOPNOTSUPP instead of EINVAL if the conntrack has no helper

This patch changes the return value if the conntrack has no helper assigned.
Instead of EINVAL, which is reserved for malformed messages, it returns 
EOPNOTSUPP.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-31 11:40:32.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-31 11:40:40.000000000 +0200
@@ -1694,7 +1694,7 @@ ctnetlink_create_expect(struct nlattr *c
 
 	if (!help || !help->helper) {
 		/* such conntrack hasn't got any helper, abort */
-		err = -EINVAL;
+		err = -EOPNOTSUPP;
 		goto out;
 	}
 

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> Moreover, someone may remove the module in the middle just after the
>>> module loading but, well, we have lost the race in the case.
>> I'd do something similar to qdiscs etc:
>>
>> - lookup helper
>> - if not found: request_module, take lock again, repeat lookup, return
>> EAGAIN if found now
>> - in the nfnetlink command handler: if ret == EAGAIN replay message
>>
>> grep for "replay" in net/ for a few examples of this. This also
>> handles the race BTW.

While reworking the patches to do it as you pointed out, I have concerns
with the current RCU locking in ctnetlink_create_conntrack. This
function calls nf_ct_helper_ext_add with GFP_KERNEL so that it may
sleep. However, we hold the read-side lock. AFAIK, this is illegal.
Therefore, whether we call it with GFP_ATOMIC or we have to perform
another lookup.

Moreover, I think that:

rcu_assign_pointer(help->helper, helper);

should be:

help->helper = rcu_dereference(helper);

since we're fetching the pointer, not publishing a new object protected
by RCU. BTW, why do we need rcu_read_unlock once the entry has been
inserted? It should be fine to release it after the helper has been
assigned.

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

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1472 bytes --]

[PATCH] Fix RCU locking in ctnetlink_create_conntrack

This patch fixes an illegal allocation with GFP_KERNEL (that may sleep) 
while holding the read-side lock. Use rcu_dereference to fetch a pointer
protected by RCU instead of rcu_assign_pointer. And release the lock once
the helper has been assigned.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-31 19:41:43.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-31 19:43:13.000000000 +0200
@@ -1154,15 +1154,16 @@ ctnetlink_create_conntrack(struct nlattr
 	rcu_read_lock();
 	helper = __nf_ct_helper_find(rtuple);
 	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+		/* we cannot sleep holding the read-side lock */
+		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 		if (help == NULL) {
 			rcu_read_unlock();
 			err = -ENOMEM;
 			goto err;
 		}
-		/* not in hash table yet so not strictly necessary */
-		rcu_assign_pointer(help->helper, helper);
+		help->helper = rcu_dereference(helper);
 	}
+	rcu_read_unlock();
 
 	/* setup master conntrack: this is a confirmed expectation */
 	if (master_ct) {
@@ -1172,7 +1173,6 @@ ctnetlink_create_conntrack(struct nlattr
 
 	add_timer(&ct->timeout);
 	nf_conntrack_hash_insert(ct);
-	rcu_read_unlock();
 
 	return 0;
 

Re: [PATCH 4/7] Helper modules load on-demand support for ctnetlink

[Pablo Neira Ayuso]

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

Pablo Neira Ayuso wrote:
> While reworking the patches to do it as you pointed out, I have concerns
> with the current RCU locking in ctnetlink_create_conntrack. This
> function calls nf_ct_helper_ext_add with GFP_KERNEL so that it may
> sleep. However, we hold the read-side lock. AFAIK, this is illegal.
> Therefore, whether we call it with GFP_ATOMIC or we have to perform
> another lookup.
> 
> Moreover, I think that:
> 
> rcu_assign_pointer(help->helper, helper);
> 
> should be:
> 
> help->helper = rcu_dereference(helper);

Sorry, this is bogus. As the comment states, we can remove completely
the rcu_assign_pointer, but not replace it with this.

> since we're fetching the pointer, not publishing a new object protected
> by RCU. BTW, why do we need rcu_read_unlock once the entry has been
> inserted? It should be fine to release it after the helper has been
> assigned.

This also, it's there to prevent preemption. As the helper removal can
modify the conntrack's pointer, the rcu protects also help section in
the conntrack.

But at least, the sleep is illegal.

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

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 909 bytes --]

[PATCH] Remove GFP_KERNEL allocation under read-side lock in ctnetlink

This patch fixes an illegal allocation with GFP_KERNEL (that may sleep) 
while holding the read-side lock.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.git.orig/net/netfilter/nf_conntrack_netlink.c	2008-07-31 19:41:43.000000000 +0200
+++ net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c	2008-07-31 23:25:39.000000000 +0200
@@ -1154,7 +1154,8 @@ ctnetlink_create_conntrack(struct nlattr
 	rcu_read_lock();
 	helper = __nf_ct_helper_find(rtuple);
 	if (helper) {
-		help = nf_ct_helper_ext_add(ct, GFP_KERNEL);
+		/* we cannot sleep holding the read-side lock */
+		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 		if (help == NULL) {
 			rcu_read_unlock();
 			err = -ENOMEM;