Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
Florian Westphalwrites: > Pablo Neira Ayuso wrote: >> On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: >> > Pablo Neira Ayuso writes: >> [...] >> > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 >> > > From: Pablo Neira Ayuso >> > > 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 >> > > --- >> > > 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, >state, _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, >state, >> _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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
Pablo Neira Ayusowrote: > On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: > > Pablo Neira Ayuso writes: > [...] > > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 > > > From: Pablo Neira Ayuso > > > 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 > > > --- > > > 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, >state, _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, >state, _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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote: > Pablo Neira Ayusowrites: [...] > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001 > > From: Pablo Neira Ayuso > > 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 > > --- > > 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, >state, _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, >state, _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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
Pablo Neira Ayusowrites: > On Mon, Oct 17, 2016 at 11:23:01AM -0400, Aaron Conole wrote: >> Pablo Neira Ayuso 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 >> > --- >> > 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, >state, _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 > 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 > --- > 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, >state, _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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
On Mon, Oct 17, 2016 at 11:23:01AM -0400, Aaron Conole wrote: > Pablo Neira Ayusowrites: > > > 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 > > --- > > 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, >state, _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 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 --- 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, >state, _entry); + hook_entry = rcu_dereference(hook_entry->next); + if (hook_entry) +next_hook: + verdict = nf_iterate(skb, >state, _entry); } switch (verdict & NF_VERDICT_MASK) { -- 2.1.4
Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
Pablo Neira Ayusowrites: > 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 > --- > 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, >state, _entry); > } ACK. I thought switch case below could have a problem, but re-checked the first nf_queue leg, and it seems okay. -Aaron -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection
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--- 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, >state, _entry); } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html