Florian Westphal <f...@strlen.de> writes:

> Pablo Neira Ayuso <pa...@netfilter.org> wrote:
>> On Mon, Oct 17, 2016 at 03:29:27PM -0400, Aaron Conole wrote:
>> > Pablo Neira Ayuso <pa...@netfilter.org> writes:
>> [...]
>> > > From c1a731c68791bcd504a7fe5d28f5f0fd59d66118 Mon Sep 17 00:00:00 2001
>> > > From: Pablo Neira Ayuso <pa...@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 <pa...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to