[PATCH 0/3] netfilter: nf_tables cleanups

[PATCH 0/3] netfilter: nf_tables cleanups

[Patrick McHardy]

These patches for -next contain some cleanups which are not directly
related to the following set patches, so I'm sending them seperately
for easier review:

 * move the reoccuring checks for skb->nf_trace to an inline function
 * consolidate similar invocations of nft_trace_packet()
 * rearrange nf_tables.h so related things are next to each other

Please apply, thanks!


Patrick McHardy (3):
      netfilter: nf_tables: minor tracing cleanups
      netfilter: nf_tables: consolidate tracing invocations
      netfilter: nf_tables: cleanup nf_tables.h

 include/net/netfilter/nf_tables.h | 174 +++++++++++++++++++-------------------
 net/netfilter/nf_tables_core.c    | 105 +++++++++++------------
 2 files changed, 138 insertions(+), 141 deletions(-)

[PATCH 1/3] netfilter: nf_tables: minor tracing cleanups

[Patrick McHardy]

The tracing code is squeezed between multiple related parts of the
evaluation code, move it out. Also add an inline wrapper for the
reoccuring test for skb->nf_trace.

Small code savings in nft_do_chain():

  nft_trace_packet          | -137
  nft_do_chain              |   -8
 2 functions changed, 145 bytes removed, diff: -145

net/netfilter/nf_tables_core.c:
  __nft_trace_packet | +137
 1 function changed, 137 bytes added, diff: +137

net/netfilter/nf_tables_core.o:
 3 functions changed, 137 bytes added, 145 bytes removed, diff: -8

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_core.c | 98 +++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 3b90eb2..074067d 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -21,6 +21,48 @@
 #include <net/netfilter/nf_tables.h>
 #include <net/netfilter/nf_log.h>
 
+enum nft_trace {
+	NFT_TRACE_RULE,
+	NFT_TRACE_RETURN,
+	NFT_TRACE_POLICY,
+};
+
+static const char *const comments[] = {
+	[NFT_TRACE_RULE]	= "rule",
+	[NFT_TRACE_RETURN]	= "return",
+	[NFT_TRACE_POLICY]	= "policy",
+};
+
+static struct nf_loginfo trace_loginfo = {
+	.type = NF_LOG_TYPE_LOG,
+	.u = {
+		.log = {
+			.level = 4,
+			.logflags = NF_LOG_MASK,
+	        },
+	},
+};
+
+static void __nft_trace_packet(const struct nft_pktinfo *pkt,
+			       const struct nft_chain *chain,
+			       int rulenum, enum nft_trace type)
+{
+	struct net *net = dev_net(pkt->in ? pkt->in : pkt->out);
+
+	nf_log_packet(net, pkt->xt.family, pkt->ops->hooknum, pkt->skb, pkt->in,
+		      pkt->out, &trace_loginfo, "TRACE: %s:%s:%s:%u ",
+		      chain->table->name, chain->name, comments[type],
+		      rulenum);
+}
+
+static inline void nft_trace_packet(const struct nft_pktinfo *pkt,
+				    const struct nft_chain *chain,
+				    int rulenum, enum nft_trace type)
+{
+	if (unlikely(pkt->skb->nf_trace))
+		__nft_trace_packet(pkt, chain, rulenum, type);
+}
+
 static void nft_cmp_fast_eval(const struct nft_expr *expr,
 			      struct nft_data data[NFT_REG_MAX + 1])
 {
@@ -66,40 +108,6 @@ struct nft_jumpstack {
 	int			rulenum;
 };
 
-enum nft_trace {
-	NFT_TRACE_RULE,
-	NFT_TRACE_RETURN,
-	NFT_TRACE_POLICY,
-};
-
-static const char *const comments[] = {
-	[NFT_TRACE_RULE]	= "rule",
-	[NFT_TRACE_RETURN]	= "return",
-	[NFT_TRACE_POLICY]	= "policy",
-};
-
-static struct nf_loginfo trace_loginfo = {
-	.type = NF_LOG_TYPE_LOG,
-	.u = {
-		.log = {
-			.level = 4,
-			.logflags = NF_LOG_MASK,
-	        },
-	},
-};
-
-static void nft_trace_packet(const struct nft_pktinfo *pkt,
-			     const struct nft_chain *chain,
-			     int rulenum, enum nft_trace type)
-{
-	struct net *net = dev_net(pkt->in ? pkt->in : pkt->out);
-
-	nf_log_packet(net, pkt->xt.family, pkt->ops->hooknum, pkt->skb, pkt->in,
-		      pkt->out, &trace_loginfo, "TRACE: %s:%s:%s:%u ",
-		      chain->table->name, chain->name, comments[type],
-		      rulenum);
-}
-
 unsigned int
 nft_do_chain(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
 {
@@ -146,8 +154,7 @@ next_rule:
 			data[NFT_REG_VERDICT].verdict = NFT_CONTINUE;
 			continue;
 		case NFT_CONTINUE:
-			if (unlikely(pkt->skb->nf_trace))
-				nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
+			nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
 			continue;
 		}
 		break;
@@ -157,16 +164,13 @@ next_rule:
 	case NF_ACCEPT:
 	case NF_DROP:
 	case NF_QUEUE:
-		if (unlikely(pkt->skb->nf_trace))
-			nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
-
+		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
 		return data[NFT_REG_VERDICT].verdict;
 	}
 
 	switch (data[NFT_REG_VERDICT].verdict) {
 	case NFT_JUMP:
-		if (unlikely(pkt->skb->nf_trace))
-			nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
+		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
 
 		BUG_ON(stackptr >= NFT_JUMP_STACK_SIZE);
 		jumpstack[stackptr].chain = chain;
@@ -176,18 +180,15 @@ next_rule:
 		chain = data[NFT_REG_VERDICT].chain;
 		goto do_chain;
 	case NFT_GOTO:
-		if (unlikely(pkt->skb->nf_trace))
-			nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
+		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
 
 		chain = data[NFT_REG_VERDICT].chain;
 		goto do_chain;
 	case NFT_RETURN:
-		if (unlikely(pkt->skb->nf_trace))
-			nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RETURN);
+		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RETURN);
 		break;
 	case NFT_CONTINUE:
-		if (unlikely(pkt->skb->nf_trace && !(chain->flags & NFT_BASE_CHAIN)))
-			nft_trace_packet(pkt, chain, ++rulenum, NFT_TRACE_RETURN);
+		nft_trace_packet(pkt, chain, ++rulenum, NFT_TRACE_RETURN);
 		break;
 	default:
 		WARN_ON(1);
@@ -201,8 +202,7 @@ next_rule:
 		goto next_rule;
 	}
 
-	if (unlikely(pkt->skb->nf_trace))
-		nft_trace_packet(pkt, basechain, -1, NFT_TRACE_POLICY);
+	nft_trace_packet(pkt, basechain, -1, NFT_TRACE_POLICY);
 
 	rcu_read_lock_bh();
 	stats = this_cpu_ptr(rcu_dereference(nft_base_chain(basechain)->stats));
-- 
2.1.0

[PATCH 2/3] netfilter: nf_tables: consolidate tracing invocations

[Patrick McHardy]

* JUMP and GOTO are equivalent except for JUMP pushing the current
  context to the stack

* RETURN and implicit RETURN (CONTINUE) are equivalent except that
  the logged rule number differs

Result:

  nft_do_chain              | -112
 1 function changed, 112 bytes removed, diff: -112

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 074067d..77165bf 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -170,26 +170,23 @@ next_rule:
 
 	switch (data[NFT_REG_VERDICT].verdict) {
 	case NFT_JUMP:
-		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
-
 		BUG_ON(stackptr >= NFT_JUMP_STACK_SIZE);
 		jumpstack[stackptr].chain = chain;
 		jumpstack[stackptr].rule  = rule;
 		jumpstack[stackptr].rulenum = rulenum;
 		stackptr++;
-		chain = data[NFT_REG_VERDICT].chain;
-		goto do_chain;
+		/* fall through */
 	case NFT_GOTO:
 		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
 
 		chain = data[NFT_REG_VERDICT].chain;
 		goto do_chain;
+	case NFT_CONTINUE:
+		rulenum++;
+		/* fall through */
 	case NFT_RETURN:
 		nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RETURN);
 		break;
-	case NFT_CONTINUE:
-		nft_trace_packet(pkt, chain, ++rulenum, NFT_TRACE_RETURN);
-		break;
 	default:
 		WARN_ON(1);
 	}
-- 
2.1.0

[PATCH 3/3] netfilter: nf_tables: cleanup nf_tables.h

[Patrick McHardy]

The transaction related definitions are squeezed in between the rule
and expression definitions, which are closely related and should be
next to each other. The transaction definitions actually don't belong
into that file at all since it defines the global objects and API and
transactions are internal to nf_tables_api, but for now simply move
them to a seperate section.

Similar, the chain types are in between a set of registration functions,
they belong to the chain section.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h | 174 +++++++++++++++++++-------------------
 1 file changed, 87 insertions(+), 87 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index decb9a0..2c6a2e2 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -409,74 +409,6 @@ struct nft_rule {
 		__attribute__((aligned(__alignof__(struct nft_expr))));
 };
 
-/**
- *	struct nft_trans - nf_tables object update in transaction
- *
- *	@list: used internally
- *	@msg_type: message type
- *	@ctx: transaction context
- *	@data: internal information related to the transaction
- */
-struct nft_trans {
-	struct list_head		list;
-	int				msg_type;
-	struct nft_ctx			ctx;
-	char				data[0];
-};
-
-struct nft_trans_rule {
-	struct nft_rule			*rule;
-};
-
-#define nft_trans_rule(trans)	\
-	(((struct nft_trans_rule *)trans->data)->rule)
-
-struct nft_trans_set {
-	struct nft_set	*set;
-	u32		set_id;
-};
-
-#define nft_trans_set(trans)	\
-	(((struct nft_trans_set *)trans->data)->set)
-#define nft_trans_set_id(trans)	\
-	(((struct nft_trans_set *)trans->data)->set_id)
-
-struct nft_trans_chain {
-	bool		update;
-	char		name[NFT_CHAIN_MAXNAMELEN];
-	struct nft_stats __percpu *stats;
-	u8		policy;
-};
-
-#define nft_trans_chain_update(trans)	\
-	(((struct nft_trans_chain *)trans->data)->update)
-#define nft_trans_chain_name(trans)	\
-	(((struct nft_trans_chain *)trans->data)->name)
-#define nft_trans_chain_stats(trans)	\
-	(((struct nft_trans_chain *)trans->data)->stats)
-#define nft_trans_chain_policy(trans)	\
-	(((struct nft_trans_chain *)trans->data)->policy)
-
-struct nft_trans_table {
-	bool		update;
-	bool		enable;
-};
-
-#define nft_trans_table_update(trans)	\
-	(((struct nft_trans_table *)trans->data)->update)
-#define nft_trans_table_enable(trans)	\
-	(((struct nft_trans_table *)trans->data)->enable)
-
-struct nft_trans_elem {
-	struct nft_set		*set;
-	struct nft_set_elem	elem;
-};
-
-#define nft_trans_elem_set(trans)	\
-	(((struct nft_trans_elem *)trans->data)->set)
-#define nft_trans_elem(trans)	\
-	(((struct nft_trans_elem *)trans->data)->elem)
-
 static inline struct nft_expr *nft_expr_first(const struct nft_rule *rule)
 {
 	return (struct nft_expr *)&rule->data[0];
@@ -544,6 +476,25 @@ enum nft_chain_type {
 	NFT_CHAIN_T_MAX
 };
 
+/**
+ * 	struct nf_chain_type - nf_tables chain type info
+ *
+ * 	@name: name of the type
+ * 	@type: numeric identifier
+ * 	@family: address family
+ * 	@owner: module owner
+ * 	@hook_mask: mask of valid hooks
+ * 	@hooks: hookfn overrides
+ */
+struct nf_chain_type {
+	const char			*name;
+	enum nft_chain_type		type;
+	int				family;
+	struct module			*owner;
+	unsigned int			hook_mask;
+	nf_hookfn			*hooks[NF_MAX_HOOKS];
+};
+
 int nft_chain_validate_dependency(const struct nft_chain *chain,
 				  enum nft_chain_type type);
 int nft_chain_validate_hooks(const struct nft_chain *chain,
@@ -630,25 +581,6 @@ struct nft_af_info {
 int nft_register_afinfo(struct net *, struct nft_af_info *);
 void nft_unregister_afinfo(struct nft_af_info *);
 
-/**
- * 	struct nf_chain_type - nf_tables chain type info
- *
- * 	@name: name of the type
- * 	@type: numeric identifier
- * 	@family: address family
- * 	@owner: module owner
- * 	@hook_mask: mask of valid hooks
- * 	@hooks: hookfn overrides
- */
-struct nf_chain_type {
-	const char			*name;
-	enum nft_chain_type		type;
-	int				family;
-	struct module			*owner;
-	unsigned int			hook_mask;
-	nf_hookfn			*hooks[NF_MAX_HOOKS];
-};
-
 int nft_register_chain_type(const struct nf_chain_type *);
 void nft_unregister_chain_type(const struct nf_chain_type *);
 
@@ -673,4 +605,72 @@ void nft_unregister_expr(struct nft_expr_type *);
 #define MODULE_ALIAS_NFT_SET() \
 	MODULE_ALIAS("nft-set")
 
+/**
+ *	struct nft_trans - nf_tables object update in transaction
+ *
+ *	@list: used internally
+ *	@msg_type: message type
+ *	@ctx: transaction context
+ *	@data: internal information related to the transaction
+ */
+struct nft_trans {
+	struct list_head		list;
+	int				msg_type;
+	struct nft_ctx			ctx;
+	char				data[0];
+};
+
+struct nft_trans_rule {
+	struct nft_rule			*rule;
+};
+
+#define nft_trans_rule(trans)	\
+	(((struct nft_trans_rule *)trans->data)->rule)
+
+struct nft_trans_set {
+	struct nft_set			*set;
+	u32				set_id;
+};
+
+#define nft_trans_set(trans)	\
+	(((struct nft_trans_set *)trans->data)->set)
+#define nft_trans_set_id(trans)	\
+	(((struct nft_trans_set *)trans->data)->set_id)
+
+struct nft_trans_chain {
+	bool				update;
+	char				name[NFT_CHAIN_MAXNAMELEN];
+	struct nft_stats __percpu	*stats;
+	u8				policy;
+};
+
+#define nft_trans_chain_update(trans)	\
+	(((struct nft_trans_chain *)trans->data)->update)
+#define nft_trans_chain_name(trans)	\
+	(((struct nft_trans_chain *)trans->data)->name)
+#define nft_trans_chain_stats(trans)	\
+	(((struct nft_trans_chain *)trans->data)->stats)
+#define nft_trans_chain_policy(trans)	\
+	(((struct nft_trans_chain *)trans->data)->policy)
+
+struct nft_trans_table {
+	bool				update;
+	bool				enable;
+};
+
+#define nft_trans_table_update(trans)	\
+	(((struct nft_trans_table *)trans->data)->update)
+#define nft_trans_table_enable(trans)	\
+	(((struct nft_trans_table *)trans->data)->enable)
+
+struct nft_trans_elem {
+	struct nft_set			*set;
+	struct nft_set_elem		elem;
+};
+
+#define nft_trans_elem_set(trans)	\
+	(((struct nft_trans_elem *)trans->data)->set)
+#define nft_trans_elem(trans)	\
+	(((struct nft_trans_elem *)trans->data)->elem)
+
 #endif /* _NET_NF_TABLES_H */
-- 
2.1.0

Re: [PATCH 0/3] netfilter: nf_tables cleanups

[Pablo Neira Ayuso]

On Tue, Mar 03, 2015 at 08:10:03PM +0000, Patrick McHardy wrote:
> These patches for -next contain some cleanups which are not directly
> related to the following set patches, so I'm sending them seperately
> for easier review:
> 
>  * move the reoccuring checks for skb->nf_trace to an inline function
>  * consolidate similar invocations of nft_trace_packet()
>  * rearrange nf_tables.h so related things are next to each other

Applied, thanks Patrick.

I'll send another net-next batch with accumulated patches just after
David gets the pending fixes. Then ask for net to go into net-next,
sorry I cannot go any faster.

Re: [PATCH 0/3] netfilter: nf_tables cleanups

[Patrick McHardy]

On 06.03, Pablo Neira Ayuso wrote:
> On Tue, Mar 03, 2015 at 08:10:03PM +0000, Patrick McHardy wrote:
> > These patches for -next contain some cleanups which are not directly
> > related to the following set patches, so I'm sending them seperately
> > for easier review:
> > 
> >  * move the reoccuring checks for skb->nf_trace to an inline function
> >  * consolidate similar invocations of nft_trace_packet()
> >  * rearrange nf_tables.h so related things are next to each other
> 
> Applied, thanks Patrick.
> 
> I'll send another net-next batch with accumulated patches just after
> David gets the pending fixes. Then ask for net to go into net-next,
> sorry I cannot go any faster.

Thanks Pablo, I expected this will take a while.