* 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.