Helper modules load on-demand support for ctnetlink

PATCH Applied Pablo Neira Ayuso 2 participants 10 messages Jul 30, 2008 – Jul 31, 2008

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2008-07-30 11:04 UTC #0 lore raw

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

[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 <[email protected]>

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

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

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

Signed-off-by Pablo Neira Ayuso <[email protected]>

net/netfilter/nf_conntrack_core.c +2 ~~-28~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
@@ -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);

net/netfilter/nf_conntrack_netlink.c +17 ~~-14~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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);

include/net/netfilter/nf_conntrack_helper.h +13 -3

Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
@@ -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);

net/netfilter/nf_conntrack_helper.c +31 -2

Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
@@ -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)
 {

Patrick McHardy

2008-07-30 11:11 UTC #1 lore raw

Quoted 32 lines · Pablo Neira Ayuso

[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 <[email protected]>
Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
net/netfilter/nf_conntrack_netlink.c +0 -4
@@ -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) {

net/netfilter/nf_conntrack_netlink.c ctnetlink_create_conntrack(struct nlattr · line 1153

Quoted 10 lines

net/netfilter/nf_conntrack_netlink.c +1 -9

-		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.

Pablo Neira Ayuso

author maintainer

2008-07-30 11:29 UTC #2 lore raw

Quoted 18 lines · Patrick McHardy

Quoted 6 lines

(quoted diff) +0 -2

@@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
     rcu_read_lock();
-    helper = __nf_ct_helper_find(rtuple);
-    if (helper) {

Review ctnetlink_create_conntrack(struct nlattr · line 1153

Quoted 10 lines

(quoted diff) +1 -9

-        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.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

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

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

Signed-off-by Pablo Neira Ayuso <[email protected]>

net/netfilter/nf_conntrack_core.c +2 ~~-28~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
@@ -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);

net/netfilter/nf_conntrack_netlink.c +19 ~~-13~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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);

include/net/netfilter/nf_conntrack_helper.h +13 -3

Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
@@ -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);

net/netfilter/nf_conntrack_helper.c +31 -2

Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
@@ -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)
 {

Pablo Neira Ayuso

author maintainer

2008-07-30 11:33 UTC #3 lore raw

Quoted 19 lines · Pablo Neira Ayuso

Quoted 17 lines · Patrick McHardy

Quoted 6 lines

(quoted diff) +0 -2

@@ -1152,17 +1150,7 @@ ctnetlink_create_conntrack(struct nlattr
 #endif
 
     rcu_read_lock();
-    helper = __nf_ct_helper_find(rtuple);
-    if (helper) {

Review ctnetlink_create_conntrack(struct nlattr · line 1153

Quoted 10 lines

(quoted diff) +1 -9

-        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.

Quoted 1 line

Attached a new version.

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

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

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

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

Signed-off-by Pablo Neira Ayuso <[email protected]>

net/netfilter/nf_conntrack_core.c +2 ~~-28~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
@@ -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);

net/netfilter/nf_conntrack_netlink.c +20 ~~-13~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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);

include/net/netfilter/nf_conntrack_helper.h +13 -3

Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
@@ -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);

net/netfilter/nf_conntrack_helper.c +31 -2

Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
@@ -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)
 {

Patrick McHardy

2008-07-30 13:35 UTC #4 lore raw

Quoted 11 lines · Pablo Neira Ayuso

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

net/netfilter/nf_conntrack_netlink.c +3 -1

@@ -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;

net/netfilter/nf_conntrack_netlink.c ctnetlink_create_expect(struct nlattr *c · line 1676

Quoted 10 lines

net/netfilter/nf_conntrack_netlink.c +10 -0

+		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?

Pablo Neira Ayuso

author maintainer

2008-07-31 08:36 UTC #5 lore raw

Quoted 17 lines · Patrick McHardy

Quoted 16 lines · Pablo Neira Ayuso

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

(quoted diff) +8 -1

@@ -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]);

Quoted 8 lines

Review ctnetlink_create_expect(struct nlattr *c · line 1676
Quoted 5 lines
(quoted diff) +5 -0
+        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.

Quoted 3 lines

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.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

[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 <[email protected]>

net/netfilter/nf_conntrack_netlink.c +29 -0

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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);

Patrick McHardy

2008-07-31 08:46 UTC #6 lore raw

Quoted 29 lines · Pablo Neira Ayuso

Quoted 15 lines · Patrick McHardy
Quoted 14 lines · Pablo Neira Ayuso
Sorry, it's wrong. Please, take this.
(quoted diff) +7 -1
@@ -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;
Quoted 8 lines
Review ctnetlink_create_expect(struct nlattr *c · line 1676
Quoted 6 lines
(quoted diff) +6 -0
+        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.

Quoted 8 lines

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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 2 lines

Please, have a look at the patch attached.

Looks fine.

Pablo Neira Ayuso

author maintainer

2008-07-31 09:44 UTC #7 lore raw

Quoted 6 lines · Patrick McHardy

Quoted 2 lines · Pablo Neira Ayuso
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:

Quoted 7 lines

- 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.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
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?

Quoted 5 lines

Yes, I think thats cleaner.
Quoted 2 lines
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.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

[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 <[email protected]>

net/netfilter/nf_conntrack_core.c +2 ~~-28~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_core.c
===================================================================
@@ -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);

include/net/netfilter/nf_conntrack_helper.h +13 -3

Index: net-next-2.6.git/include/net/netfilter/nf_conntrack_helper.h
===================================================================
@@ -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);

net/netfilter/nf_conntrack_helper.c +31 -2

Index: net-next-2.6.git/net/netfilter/nf_conntrack_helper.c
===================================================================
@@ -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)
 {

net/netfilter/nf_conntrack_netlink.c +4 ~~-12~~

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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 */

[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 <[email protected]>

net/netfilter/nf_conntrack_netlink.c +1 -1

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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;
 	}

Pablo Neira Ayuso

author maintainer

2008-07-31 17:51 UTC #8 lore raw

Quoted 6 lines · Pablo Neira Ayuso

Quoted 5 lines · Patrick McHardy
Quoted 2 lines · Pablo Neira Ayuso
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:

Quoted 7 lines

Quoted 7 lines
- 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.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

[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 <[email protected]>

net/netfilter/nf_conntrack_netlink.c +4 -4

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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;

Pablo Neira Ayuso

author maintainer

2008-07-31 21:27 UTC #9 lore raw

Quoted 7 lines · Pablo Neira Ayuso

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Signature · 1 line

"Los honestos son inadaptados sociales" -- Les Luthiers

[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 <[email protected]>

net/netfilter/nf_conntrack_netlink.c +2 -1

Index: net-next-2.6.git/net/netfilter/nf_conntrack_netlink.c
===================================================================
@@ -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;