* Possible netfilter-related memory corruption in 2.6.37
@ 2011-02-14 14:58 Avi Kivity
2011-02-14 15:11 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Avi Kivity @ 2011-02-14 14:58 UTC (permalink / raw)
To: netfilter-devel; +Cc: Marcelo Tosatti, nicolas prochazka, KVM list
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 14:58 Possible netfilter-related memory corruption in 2.6.37 Avi Kivity
@ 2011-02-14 15:11 ` Eric Dumazet
2011-02-14 15:18 ` Jan Engelhardt
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2011-02-14 15:11 UTC (permalink / raw)
To: Avi Kivity
Cc: netfilter-devel, Marcelo Tosatti, nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 15:11 ` Eric Dumazet
@ 2011-02-14 15:18 ` Jan Engelhardt
2011-02-14 15:50 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2011-02-14 15:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: Avi Kivity, netfilter-devel, Marcelo Tosatti, nicolas prochazka,
KVM list, netdev
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 15:18 ` Jan Engelhardt
@ 2011-02-14 15:50 ` Eric Dumazet
2011-02-14 16:24 ` Patrick McHardy
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2011-02-14 15:50 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Avi Kivity, netfilter-devel, Marcelo Tosatti, nicolas prochazka,
KVM list, netdev
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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 15:50 ` Eric Dumazet
@ 2011-02-14 16:24 ` Patrick McHardy
2011-02-14 16:29 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Patrick McHardy @ 2011-02-14 16:24 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]
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?
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 640 bytes --]
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;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 16:24 ` Patrick McHardy
@ 2011-02-14 16:29 ` Eric Dumazet
2011-02-14 16:37 ` Patrick McHardy
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2011-02-14 16:29 UTC (permalink / raw)
To: Patrick McHardy
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 16:29 ` Eric Dumazet
@ 2011-02-14 16:37 ` Patrick McHardy
2011-02-14 16:48 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Patrick McHardy @ 2011-02-14 16:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 16:37 ` Patrick McHardy
@ 2011-02-14 16:48 ` Eric Dumazet
2011-02-14 16:52 ` Patrick McHardy
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2011-02-14 16:48 UTC (permalink / raw)
To: Patrick McHardy
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 16:48 ` Eric Dumazet
@ 2011-02-14 16:52 ` Patrick McHardy
2011-02-18 18:37 ` Patrick McHardy
0 siblings, 1 reply; 11+ messages in thread
From: Patrick McHardy @ 2011-02-14 16:52 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-14 16:52 ` Patrick McHardy
@ 2011-02-18 18:37 ` Patrick McHardy
2011-02-18 19:14 ` Eric Dumazet
0 siblings, 1 reply; 11+ messages in thread
From: Patrick McHardy @ 2011-02-18 18:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
[-- Attachment #1: Type: text/plain, Size: 391 bytes --]
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.
[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 1009 bytes --]
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) {
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Possible netfilter-related memory corruption in 2.6.37
2011-02-18 18:37 ` Patrick McHardy
@ 2011-02-18 19:14 ` Eric Dumazet
0 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2011-02-18 19:14 UTC (permalink / raw)
To: Patrick McHardy
Cc: Jan Engelhardt, Avi Kivity, netfilter-devel, Marcelo Tosatti,
nicolas prochazka, KVM list, netdev
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-02-18 19:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-14 14:58 Possible netfilter-related memory corruption in 2.6.37 Avi Kivity
2011-02-14 15:11 ` Eric Dumazet
2011-02-14 15:18 ` Jan Engelhardt
2011-02-14 15:50 ` Eric Dumazet
2011-02-14 16:24 ` Patrick McHardy
2011-02-14 16:29 ` Eric Dumazet
2011-02-14 16:37 ` Patrick McHardy
2011-02-14 16:48 ` Eric Dumazet
2011-02-14 16:52 ` Patrick McHardy
2011-02-18 18:37 ` Patrick McHardy
2011-02-18 19:14 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.