Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-18 Thread Eric Dumazet 
Le vendredi 18 février 2011 à 19:37 +0100, Patrick McHardy a écrit :
> Am 14.02.2011 17:52, schrieb Patrick McHardy:
> > Am 14.02.2011 17:48, schrieb Eric Dumazet:
> >> 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 kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-18 Thread Patrick McHardy 
Am 14.02.2011 17:52, schrieb Patrick McHardy:
> Am 14.02.2011 17:48, schrieb Eric Dumazet:
>> 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.


diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 74aebed..834bb07 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -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) {

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Patrick McHardy 
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 
>>
>> 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 kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread 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 
> 
> 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 kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Patrick McHardy 
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 

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

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Eric Dumazet 
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 



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Patrick McHardy 
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?

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;

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread 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;



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Jan Engelhardt 
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread Eric Dumazet 
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
> 

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 kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Possible netfilter-related memory corruption in 2.6.37

2011-02-14 Thread 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

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html