[PATCH] netfilter: nf_tables: fix oops when using gotos

[PATCH] netfilter: nf_tables: fix oops when using gotos

[Pablo Neira Ayuso]

Fix an oops if you use the "goto" operation, you will end up in
non-base chain with no counters and no default policy. While at
it, uninlined nft_chain_stats() since it doesn't make sense to
optimize a debugging facility that is rarely used.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
I noticed this when testing the compat layer.

@Patrick: When working on this I noticed I accidentally has skipped
this patch: http://www.spinics.net/lists/netfilter-devel/msg29828.html
I have included this change in this fix. Sorry for that.

 net/netfilter/nf_tables_core.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 0d879fc..bdee0ae 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -67,18 +67,24 @@ struct nft_jumpstack {
 	int			rulenum;
 };
 
-static inline void
+static int
 nft_chain_stats(const struct nft_chain *this, const struct nft_pktinfo *pkt,
 		struct nft_jumpstack *jumpstack, unsigned int stackptr)
 {
 	struct nft_stats __percpu *stats;
 	const struct nft_chain *chain = stackptr ? jumpstack[0].chain : this;
 
+	/* You can reach this through goto chain */
+	if (!(chain->flags & NFT_BASE_CHAIN))
+		return NF_ACCEPT;
+
 	rcu_read_lock_bh();
 	stats = rcu_dereference(nft_base_chain(chain)->stats);
 	__this_cpu_inc(stats->pkts);
 	__this_cpu_add(stats->bytes, pkt->skb->len);
 	rcu_read_unlock_bh();
+
+	return nft_base_chain(chain)->policy;
 }
 
 enum nft_trace {
@@ -209,12 +215,11 @@ next_rule:
 		rulenum = jumpstack[stackptr].rulenum;
 		goto next_rule;
 	}
-	nft_chain_stats(chain, pkt, jumpstack, stackptr);
 
 	if (unlikely(pkt->skb->nf_trace))
 		nft_trace_packet(pkt, chain, ++rulenum, NFT_TRACE_POLICY);
 
-	return nft_base_chain(chain)->policy;
+	return nft_chain_stats(chain, pkt, jumpstack, stackptr);
 }
 EXPORT_SYMBOL_GPL(nft_do_chain);
 
-- 
1.7.10.4

Re: [PATCH] netfilter: nf_tables: fix oops when using gotos

[Pablo Neira Ayuso]

On Fri, Feb 07, 2014 at 05:18:51PM +0100, Pablo Neira Ayuso wrote:
> Fix an oops if you use the "goto" operation, you will end up in
> non-base chain with no counters and no default policy. While at
> it, uninlined nft_chain_stats() since it doesn't make sense to
> optimize a debugging facility that is rarely used.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> I noticed this when testing the compat layer.
> 
> @Patrick: When working on this I noticed I accidentally has skipped
> this patch: http://www.spinics.net/lists/netfilter-devel/msg29828.html
> I have included this change in this fix. Sorry for that.

Oops, unlined the wrong function...

Re: [PATCH] netfilter: nf_tables: fix oops when using gotos

[Pablo Neira Ayuso]

On Fri, Feb 07, 2014 at 05:23:30PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 07, 2014 at 05:18:51PM +0100, Pablo Neira Ayuso wrote:
> > Fix an oops if you use the "goto" operation, you will end up in
> > non-base chain with no counters and no default policy. While at
> > it, uninlined nft_chain_stats() since it doesn't make sense to
> > optimize a debugging facility that is rarely used.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > I noticed this when testing the compat layer.
> > 
> > @Patrick: When working on this I noticed I accidentally has skipped
> > this patch: http://www.spinics.net/lists/netfilter-devel/msg29828.html
> > I have included this change in this fix. Sorry for that.
> 
> Oops, unlined the wrong function...

I'm going to push this leftover fix to nf:

http://patchwork.ozlabs.org/patch/308930/

Will resend a v2 to fix the goto issue.

Re: [PATCH] netfilter: nf_tables: fix oops when using gotos

[Patrick McHardy]

On Fri, Feb 07, 2014 at 05:26:58PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 07, 2014 at 05:23:30PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Feb 07, 2014 at 05:18:51PM +0100, Pablo Neira Ayuso wrote:
> > > Fix an oops if you use the "goto" operation, you will end up in
> > > non-base chain with no counters and no default policy. While at
> > > it, uninlined nft_chain_stats() since it doesn't make sense to
> > > optimize a debugging facility that is rarely used.
> > > 
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > > I noticed this when testing the compat layer.
> > > 
> > > @Patrick: When working on this I noticed I accidentally has skipped
> > > this patch: http://www.spinics.net/lists/netfilter-devel/msg29828.html
> > > I have included this change in this fix. Sorry for that.
> > 
> > Oops, unlined the wrong function...
> 
> I'm going to push this leftover fix to nf:
> 
> http://patchwork.ozlabs.org/patch/308930/
> 
> Will resend a v2 to fix the goto issue.

Thanks! I was also considering moving this to jump labels and enable
the label on nft meta set trace. Not sure if its worth it though,
will do some benchmarking at some point ...