[PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Pablo Neira Ayuso]

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);
 	}
-- 
2.1.4

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Aaron Conole]

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.

-Aaron

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

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.

[-- Attachment #2: 0001-netfilter-nf_queue-don-t-re-enter-same-hook-on-packe.patch --]
[-- Type: text/x-diff, Size: 1321 bytes --]

>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

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Aaron Conole]

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

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Pablo Neira Ayuso]

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?

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Florian Westphal]

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.

Re: [PATCH nf,v2] netfilter: nf_queue: don't re-enter same hook on packet reinjection

[Aaron Conole]

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.