Patchwork Possible netfilter-related memory corruption in 2.6.37

login
register
mail settings
Submitter Patrick McHardy
Date Feb. 14, 2011, 4:24 p.m.
Message ID <4D595745.7070505@trash.net>
Download mbox | patch
Permalink /patch/83115/
State Accepted
Delegated to: David Miller
Headers show

Comments

Patrick McHardy - Feb. 14, 2011, 4:24 p.m.
Am 14.02.2011 16:50, schrieb Eric Dumazet:
> Le lundi 14 février 2011 à 16:18 +0100, Jan Engelhardt a écrit :
>> On Monday 2011-02-14 16:11, Eric Dumazet wrote:
>>
>>> Le lundi 14 février 2011 à 16:58 +0200, Avi Kivity a écrit :
>>>> 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
>>
>> 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;

Yeah, that definitely looks wrong. How about this instead?
Eric Dumazet - Feb. 14, 2011, 4:29 p.m.
Le lundi 14 février 2011 à 17:24 +0100, Patrick McHardy a écrit :
> Am 14.02.2011 16:50, schrieb Eric Dumazet:
> > Le lundi 14 février 2011 à 16:18 +0100, Jan Engelhardt a écrit :
> >> On Monday 2011-02-14 16:11, Eric Dumazet wrote:
> >>
> >>> Le lundi 14 février 2011 à 16:58 +0200, Avi Kivity a écrit :
> >>>> 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
> >>
> >> 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;
> 
> Yeah, that definitely looks wrong. How about this instead?
> 

This patch seems fine to me, thanks !

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy - Feb. 14, 2011, 4:37 p.m.
Am 14.02.2011 17:29, schrieb Eric Dumazet:
> Le lundi 14 février 2011 à 17:24 +0100, Patrick McHardy a écrit :
>>> 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 <eric.dumazet@gmail.com>

THanks Eric, I've queued the patch for 2.6.38.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Feb. 14, 2011, 4:48 p.m.
Le lundi 14 février 2011 à 17:37 +0100, Patrick McHardy a écrit :
> Am 14.02.2011 17:29, schrieb Eric Dumazet:
> > Le lundi 14 février 2011 à 17:24 +0100, Patrick McHardy a écrit :
> >>> 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 <eric.dumazet@gmail.com>
> 
> 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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy - Feb. 14, 2011, 4:52 p.m.
Am 14.02.2011 17:48, schrieb Eric Dumazet:
> Le lundi 14 février 2011 à 17:37 +0100, Patrick McHardy a écrit :
>> Am 14.02.2011 17:29, schrieb Eric Dumazet:
>>> Le lundi 14 février 2011 à 17:24 +0100, Patrick McHardy a écrit :
>>>>> 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 <eric.dumazet@gmail.com>
>>
>> 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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 1e00bf7..899b71c 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -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;