| Message ID | 20161017170320.GA5538@salvia |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Pablo Neira Ayuso <pablo@netfilter.org> writes: > On Mon, Oct 17, 2016 at 11:23:01AM -0400, Aaron Conole wrote: >> Pablo Neira Ayuso <pablo@netfilter.org> writes: >> >> > Make sure we skip the current hook from where the packet was enqueued, >> > otherwise the packets gets enqueued over and over again. >> > >> > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") >> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> >> > --- >> > v2: Make sure next hook is non-null, otherwise we are at the end of the >> > hook list and we can skip nf_iterate(). >> > >> > net/netfilter/nf_queue.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c >> > index 96964a0070e1..691e713d70f5 100644 >> > --- a/net/netfilter/nf_queue.c >> > +++ b/net/netfilter/nf_queue.c >> > @@ -185,8 +185,9 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) >> > } >> > >> > entry->state.thresh = INT_MIN; >> > + hook_entry = rcu_dereference(hook_entry->next); >> > >> > - if (verdict == NF_ACCEPT) { >> > + if (hook_entry && verdict == NF_ACCEPT) { >> > next_hook: >> > verdict = nf_iterate(skb, &entry->state, &hook_entry); >> > } >> >> ACK. I thought switch case below could have a problem, but re-checked >> the first nf_queue leg, and it seems okay. > > Argh, still not right. If we get a NF_QUEUE verdict to re-enqueue > again, then hook_entry may become NULL. > > switch (verdict & NF_VERDICT_MASK) { > case NF_ACCEPT: > case NF_STOP: > local_bh_disable(); > entry->state.okfn(entry->state.net, entry->state.sk, skb); > local_bh_enable(); > break; > case NF_QUEUE: > RCU_INIT_POINTER(entry->state.hook_entries, hook_entry); <-- > > Attaching new patch. > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 > From: Pablo Neira Ayuso <pablo@netfilter.org> > Date: Thu, 13 Oct 2016 08:14:03 +0200 > Subject: [PATCH nf,v3] netfilter: nf_queue: don't re-enter same hook on packet > reinjection > > If the packet is accepted, we have to skip the current hook from where > the packet was enqueued. Thus, we can emulate the previous > list_for_each_entry_continue() behaviour happening from nf_reinject(), > otherwise the packets gets enqueued over and over again. > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > net/netfilter/nf_queue.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > index 96964a0070e1..0b5ac3c9c2bc 100644 > --- a/net/netfilter/nf_queue.c > +++ b/net/netfilter/nf_queue.c > @@ -187,8 +187,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) > entry->state.thresh = INT_MIN; > > if (verdict == NF_ACCEPT) { > - next_hook: > - verdict = nf_iterate(skb, &entry->state, &hook_entry); > + hook_entry = rcu_dereference(hook_entry->next); > + if (hook_entry) > +next_hook: Should the above two lines be transposed to this? next_hook: if (hook_entry) Sorry if I'm misunderstanding it. Too many special cases for my tiny brain... -Aaron -- 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
On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: > Pablo Neira Ayuso <pablo@netfilter.org> writes: [...] > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 > > From: Pablo Neira Ayuso <pablo@netfilter.org> > > Date: Thu, 13 Oct 2016 08:14:03 +0200 > > Subject: [PATCH nf,v3] netfilter: nf_queue: don't re-enter same hook on packet > > reinjection > > > > If the packet is accepted, we have to skip the current hook from where > > the packet was enqueued. Thus, we can emulate the previous > > list_for_each_entry_continue() behaviour happening from nf_reinject(), > > otherwise the packets gets enqueued over and over again. > > > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > net/netfilter/nf_queue.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > > index 96964a0070e1..0b5ac3c9c2bc 100644 > > --- a/net/netfilter/nf_queue.c > > +++ b/net/netfilter/nf_queue.c > > @@ -187,8 +187,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) > > entry->state.thresh = INT_MIN; > > > > if (verdict == NF_ACCEPT) { > > - next_hook: > > - verdict = nf_iterate(skb, &entry->state, &hook_entry); > > + hook_entry = rcu_dereference(hook_entry->next); > > + if (hook_entry) > > +next_hook: > > Should the above two lines be transposed to this? > > next_hook: > if (hook_entry) > > Sorry if I'm misunderstanding it. Too many special cases for my tiny > brain... Right, my patch is still not correct. I think this should be it: if (verdict == NF_ACCEPT) { next_hook: hook_entry = rcu_dereference(hook_entry->next); if (hook_entry) verdict = nf_iterate(skb, &entry->state, &hook_entry); So we jump to "next_hook" in case of NF_QUEUE verdict with bypass flag set on. In that case, we need to continue just after the current hook entry to emulate the behaviour that we previously have via list_for_each_entry_continue(). This NF_QUEUE handling is also broken from nf_hook_slow() path, right? -- 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
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> writes: > [...] > > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 > > > From: Pablo Neira Ayuso <pablo@netfilter.org> > > > Date: Thu, 13 Oct 2016 08:14:03 +0200 > > > Subject: [PATCH nf,v3] netfilter: nf_queue: don't re-enter same hook on packet > > > reinjection > > > > > > If the packet is accepted, we have to skip the current hook from where > > > the packet was enqueued. Thus, we can emulate the previous > > > list_for_each_entry_continue() behaviour happening from nf_reinject(), > > > otherwise the packets gets enqueued over and over again. > > > > > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > --- > > > net/netfilter/nf_queue.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c > > > index 96964a0070e1..0b5ac3c9c2bc 100644 > > > --- a/net/netfilter/nf_queue.c > > > +++ b/net/netfilter/nf_queue.c > > > @@ -187,8 +187,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) > > > entry->state.thresh = INT_MIN; > > > > > > if (verdict == NF_ACCEPT) { > > > - next_hook: > > > - verdict = nf_iterate(skb, &entry->state, &hook_entry); > > > + hook_entry = rcu_dereference(hook_entry->next); > > > + if (hook_entry) > > > +next_hook: > > > > Should the above two lines be transposed to this? > > > > next_hook: > > if (hook_entry) > > > > Sorry if I'm misunderstanding it. Too many special cases for my tiny > > brain... > > Right, my patch is still not correct. > > I think this should be it: > > if (verdict == NF_ACCEPT) { > next_hook: > hook_entry = rcu_dereference(hook_entry->next); > if (hook_entry) > verdict = nf_iterate(skb, &entry->state, &hook_entry); > > So we jump to "next_hook" in case of NF_QUEUE verdict with bypass flag > set on. In that case, we need to continue just after the current hook > entry to emulate the behaviour that we previously have via > list_for_each_entry_continue(). > > This NF_QUEUE handling is also broken from nf_hook_slow() path, right? Yes. As you already indicate, list_for_each_entry_continue() resumes after the current elem, this isn't true anymore. So for nf_queue we need to move to hook_entry->next in ACCEPT case, and, for nf_hook_slow, we need to do the same when hitting (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS)) goto next_hook; branch. -- 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
Florian Westphal <fw@strlen.de> writes: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: >> > Pablo Neira Ayuso <pablo@netfilter.org> writes: >> [...] >> > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 >> > > From: Pablo Neira Ayuso <pablo@netfilter.org> >> > > Date: Thu, 13 Oct 2016 08:14:03 +0200 >> > > Subject: [PATCH nf,v3] netfilter: nf_queue: don't re-enter same hook on packet >> > > reinjection >> > > >> > > If the packet is accepted, we have to skip the current hook from where >> > > the packet was enqueued. Thus, we can emulate the previous >> > > list_for_each_entry_continue() behaviour happening from nf_reinject(), >> > > otherwise the packets gets enqueued over and over again. >> > > >> > > Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list") >> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> >> > > --- >> > > net/netfilter/nf_queue.c | 6 ++++-- >> > > 1 file changed, 4 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c >> > > index 96964a0070e1..0b5ac3c9c2bc 100644 >> > > --- a/net/netfilter/nf_queue.c >> > > +++ b/net/netfilter/nf_queue.c >> > > @@ -187,8 +187,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict) >> > > entry->state.thresh = INT_MIN; >> > > >> > > if (verdict == NF_ACCEPT) { >> > > - next_hook: >> > > - verdict = nf_iterate(skb, &entry->state, &hook_entry); >> > > + hook_entry = rcu_dereference(hook_entry->next); >> > > + if (hook_entry) >> > > +next_hook: >> > >> > Should the above two lines be transposed to this? >> > >> > next_hook: >> > if (hook_entry) >> > >> > Sorry if I'm misunderstanding it. Too many special cases for my tiny >> > brain... >> >> Right, my patch is still not correct. >> >> I think this should be it: >> >> if (verdict == NF_ACCEPT) { >> next_hook: >> hook_entry = rcu_dereference(hook_entry->next); >> if (hook_entry) >> verdict = nf_iterate(skb, &entry->state, &hook_entry); >> Yes. >> So we jump to "next_hook" in case of NF_QUEUE verdict with bypass flag >> set on. In that case, we need to continue just after the current hook >> entry to emulate the behaviour that we previously have via >> list_for_each_entry_continue(). >> >> This NF_QUEUE handling is also broken from nf_hook_slow() path, right? > > Yes. As you already indicate, list_for_each_entry_continue() resumes > after the current elem, this isn't true anymore. > > So for nf_queue we need to move to hook_entry->next in ACCEPT case, > and, for nf_hook_slow, we need to do the same when hitting > > (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS)) > goto next_hook; > > branch. Right. That looks correct. -- 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
From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 13 Oct 2016 08:14:03 +0200
Subject: [PATCH nf,v3] netfilter: nf_queue: don't re-enter same hook on packet
reinjection
If the packet is accepted, we have to skip the current hook from where
the packet was enqueued. Thus, we can emulate the previous
list_for_each_entry_continue() behaviour happening from nf_reinject(),
otherwise the packets gets enqueued over and over again.
Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_queue.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 96964a0070e1..0b5ac3c9c2bc 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -187,8 +187,10 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
entry->state.thresh = INT_MIN;
if (verdict == NF_ACCEPT) {
- next_hook:
- verdict = nf_iterate(skb, &entry->state, &hook_entry);
+ hook_entry = rcu_dereference(hook_entry->next);
+ if (hook_entry)
+next_hook:
+ verdict = nf_iterate(skb, &entry->state, &hook_entry);
}
switch (verdict & NF_VERDICT_MASK) {
--
2.1.4