Re: Oops in nf_nat_core.c:find_appropriate_src(), kernel 2.6.25.4

[Patrick McHardy <kaber@trash.net>]

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

Patrick McHardy wrote:
> Chuck Ebbert wrote:
>> Reported at https://bugzilla.redhat.com/show_bug.cgi?id=449315
>>
>> In find_appropriate_src():
>>
>>         hlist_for_each_entry_rcu(nat, n, &bysource[h], bysource) {
>>                 ct = nat->ct;
>>                 if (same_src(ct, tuple)) {
>>
>> Dereference of ct in same_src() causes the oops. This only seems to
>> happen on heavily loaded firewall machines. Kernel 2.6.24.7 works.
>>
>> The reporter identifies commit 4d354c5782dc352cec187845d17eedc2c2bfcf67
>> ("[NETFILTER]: nf_nat: use RCU for bysource hash") as a possible cause
>> of the problem.
> 
> We have a similar looking report, but that one also affects 2.6.24:
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=10875
> 
> Anyways, does this patch help? When reallocating storage
> for a conntrack, it is replaced in the list before assigning
> the nat->ct pointer.


I'm afraid we also need this one on top - when reallocating
an extension, we must not free the old storage since it may
still be used in a RCU read side.



[-- Attachment #2: x2 --]
[-- Type: text/plain, Size: 1400 bytes --]

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index f736e84..f80c0ed 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -15,6 +15,7 @@ enum nf_ct_ext_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
+	struct rcu_head rcu;
 	u8 offset[NF_CT_EXT_NUM];
 	u8 len;
 	char data[0];
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index bcc19fa..90d4a74 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
 	return (void *)(*ext) + off;
 }
 
+static void __nf_ct_ext_destroy_rcu(struct rcu_head *head)
+{
+	struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
+	kfree(ext);
+}
+
 void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {
 	struct nf_ct_ext *new;
@@ -106,7 +113,7 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
-		kfree(ct->ext);
+		call_rcu(&ct->ext->rcu, __nf_ct_ext_destroy_rcu);
 		ct->ext = new;
 	}