Skip to contentSkip to conversation

Possible netfilter-related memory corruption in 2.6.37

DISCUSSION Avi Kivity 4 participants 11 messages Feb 14, 2011 – Feb 18, 2011

Subsystems (4)
Networking Network drivers KVM netfilter
lore
Share & tools
lore

Avi Kivity

author
#0 lore raw

We see severe memory corruption in kvm while used in conjunction with bridge/netfilter. Enabling slab debugging points the finger at a netfilter chain invoked from the bridge code.

Can someone take a look?

https://bugzilla.kernel.org/show_bug.cgi?id=27052

Signature · 1 line
error compiling committee.c: too many arguments to function
#1 lore raw
Quoted 8 lines · Avi Kivity

We see severe memory corruption in kvm while used in conjunction with bridge/netfilter. Enabling slab debugging points the finger at a netfilter chain invoked from the bridge code.

Can someone take a look?

https://bugzilla.kernel.org/show_bug.cgi?id=27052

CC netdev

Does a revert of commit ca44ac386181ba7 help a bit ?

(net: don't reallocate skb->head unless the current one hasn't the needed extra size or is shared)

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 7 lines · Jan Engelhardt
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · Avi Kivity

We see severe memory corruption in kvm while used in conjunction with bridge/netfilter. Enabling slab debugging points the finger at a netfilter chain invoked from the bridge code.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Can someone take a look?

https://bugzilla.kernel.org/show_bug.cgi?id=27052

Maybe looks familiar to https://lkml.org/lkml/2011/2/3/147

Are you sure Jan ?

IMHO it looks like in your case, a NULL ->hook() is called, from nf_iterate()

BTW, list_for_each_continue_rcu() really should be converted to list_for_each_entry_continue_rcu()

This is a bit ugly :

list_for_each_continue_rcu(*i, head) {
	struct nf_hook_ops *elem = (struct nf_hook_ops *)*i;

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

#4 lore raw

Am 14.02.2011 16:50, schrieb Eric Dumazet:

Quoted 24 lines · Jan Engelhardt
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Avi Kivity
Quoted 4 lines

We see severe memory corruption in kvm while used in conjunction with bridge/netfilter. Enabling slab debugging points the finger at a netfilter chain invoked from the bridge code.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Can someone take a look?

https://bugzilla.kernel.org/show_bug.cgi?id=27052

Maybe looks familiar to https://lkml.org/lkml/2011/2/3/147

Are you sure Jan ?

IMHO it looks like in your case, a NULL ->hook() is called, from nf_iterate()

BTW, list_for_each_continue_rcu() really should be converted to list_for_each_entry_continue_rcu()

This is a bit ugly :

Quoted 7 lines
list_for_each_continue_rcu(*i, head) {
	struct nf_hook_ops *elem = (struct nf_hook_ops *)*i;

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

Yeah, that definitely looks wrong. How about this instead?

net/netfilter/core.c +2 -1
index 1e00bf7..899b71c 100644
@@ -133,6 +133,7 @@ unsigned int nf_iterate(struct list_head *head,
 
 		/* Optimization: we don't need to hold module
 		   reference here, since function can't sleep. --RR */
+repeat:
 		verdict = elem->hook(hook, skb, indev, outdev, okfn);
 		if (verdict != NF_ACCEPT) {
 #ifdef CONFIG_NETFILTER_DEBUG

@@ -145,7 +146,7 @@ unsigned int nf_iterate(struct list_head *head,
 #endif
 			if (verdict != NF_REPEAT)
 				return verdict;
-			*i = (*i)->prev;
+			goto repeat;
 		}
 	}
 	return NF_ACCEPT;
Acked-by
#5 lore raw
Quoted 28 lines · Patrick McHardy

Am 14.02.2011 16:50, schrieb Eric Dumazet:

Quoted 27 lines · Jan Engelhardt
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Avi Kivity
Quoted 4 lines

We see severe memory corruption in kvm while used in conjunction with bridge/netfilter. Enabling slab debugging points the finger at a netfilter chain invoked from the bridge code.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Can someone take a look?

https://bugzilla.kernel.org/show_bug.cgi?id=27052

Maybe looks familiar to https://lkml.org/lkml/2011/2/3/147

Are you sure Jan ?

IMHO it looks like in your case, a NULL ->hook() is called, from nf_iterate()

BTW, list_for_each_continue_rcu() really should be converted to list_for_each_entry_continue_rcu()

This is a bit ugly :

list_for_each_continue_rcu(*i, head) {
	struct nf_hook_ops *elem = (struct nf_hook_ops *)*i;
Quoted 7 lines
Quoted 4 lines

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

Yeah, that definitely looks wrong. How about this instead?

This patch seems fine to me, thanks !

Acked-by Eric Dumazet <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

Am 14.02.2011 17:29, schrieb Eric Dumazet:

Quoted 12 lines · Patrick McHardy
Quoted 7 lines
Quoted 4 lines

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

Yeah, that definitely looks wrong. How about this instead?

This patch seems fine to me, thanks !

Acked-by Eric Dumazet <[email protected]>

THanks Eric, I've queued the patch for 2.6.38.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 7 lines · Patrick McHardy

Am 14.02.2011 17:29, schrieb Eric Dumazet:

Quoted 6 lines · Patrick McHardy
Quoted 5 lines
Quoted 4 lines

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Yeah, that definitely looks wrong. How about this instead?

This patch seems fine to me, thanks !

Acked-by Eric Dumazet <[email protected]>

THanks Eric, I've queued the patch for 2.6.38.

I am not sure, but I guess nf_reinject() needs a fix too ;)

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw

Am 14.02.2011 17:48, schrieb Eric Dumazet:

Quoted 11 lines · Patrick McHardy
Quoted 10 lines

Am 14.02.2011 17:29, schrieb Eric Dumazet:

Quoted 9 lines · Patrick McHardy
Quoted 7 lines
Quoted 4 lines

Also, I wonder if RCU rules are respected in nf_iterate(). For example this line is really suspicious :

*i = (*i)->prev;

Yeah, that definitely looks wrong. How about this instead?

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

This patch seems fine to me, thanks !

Acked-by Eric Dumazet <[email protected]>

THanks Eric, I've queued the patch for 2.6.38.

I am not sure, but I guess nf_reinject() needs a fix too ;)

I agree. That one looks uglier though, I guess we'll have to iterate through all hooks to note the previous one.

I'll take care of that once Dave has pulled the last fix
since I already sent out the pull request.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw

Am 14.02.2011 17:52, schrieb Patrick McHardy:

Quoted 5 lines

Am 14.02.2011 17:48, schrieb Eric Dumazet:

Quoted 1 line

I am not sure, but I guess nf_reinject() needs a fix too ;)

I agree. That one looks uglier though, I guess we'll have to iterate through all hooks to note the previous one.

How about this? Unfortunately I don't think we can avoid iterating through all hooks without violating RCU rules.

net/netfilter/nf_queue.c +16 -2
index 74aebed..834bb07 100644
@@ -235,6 +235,7 @@ int nf_queue(struct sk_buff *skb,
 void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 {
 	struct sk_buff *skb = entry->skb;
+	struct nf_hook_ops *i, *prev;
 	struct list_head *elem = &entry->elem->list;
 	const struct nf_afinfo *afinfo;
 

@@ -244,8 +245,21 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 
 	/* Continue traversal iff userspace said ok... */
 	if (verdict == NF_REPEAT) {
-		elem = elem->prev;
-		verdict = NF_ACCEPT;
+		prev = NULL;
+		list_for_each_entry_rcu(i, &nf_hooks[entry->pf][entry->hook],
+					list) {
+			if (&i->list == elem)
+				break;
+			prev = i;
+		}
+
+		if (prev == NULL ||
+		    &i->list == &nf_hooks[entry->pf][entry->hook])
+			verdict = NF_DROP;
+		else {
+			elem = &prev->list;
+			verdict = NF_ACCEPT;
+		}
 	}
 
 	if (verdict == NF_ACCEPT) {
#10 lore raw
Quoted 11 lines · Patrick McHardy

Am 14.02.2011 17:52, schrieb Patrick McHardy:

Quoted 5 lines

Am 14.02.2011 17:48, schrieb Eric Dumazet:

Quoted 1 line

I am not sure, but I guess nf_reinject() needs a fix too ;)

I agree. That one looks uglier though, I guess we'll have to iterate through all hooks to note the previous one.

How about this? Unfortunately I don't think we can avoid iterating through all hooks without violating RCU rules.

       /* Continue traversal iff userspace said ok... */
        if (verdict == NF_REPEAT) {
-               elem = elem->prev;
-               verdict = NF_ACCEPT;
+               prev = NULL;
+               list_for_each_entry_rcu(i,

&nf_hooks[entry->pf][entry->hook],
+ list) {
+ if (&i->list == elem)
+ break;
+ prev = i;

Hmm... what happens if "elem" was the first elem in list ?

We exit with prev = NULL --> NF_DROP ?

I must miss something...

+ }
+
+ if (prev == NULL ||
+ &i->list == &nf_hooks[entry->pf][entry->hook])
+ verdict = NF_DROP;
+ else {
+ elem = &prev->list;
+ verdict = NF_ACCEPT;
+ }

        }

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html