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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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