[PATCH 0/5] netfilter: nf_tables: assorted patches

[PATCH 0/5] netfilter: nf_tables: assorted patches

[Patrick McHardy]

Hi Pablo,

following are five more or less assorted patches for nf_tables:

* fix for a minor locking issue in nft_rbtree
* move the struct net pointer from the chain to the base chain
* check for invalid NFT_SET_ELEM_INTERVAL_END centrally
* restruct struct nft_hash as preparation for following patches
* minor indentation cleanup for nft_hash

I'm sending these patches seperately from the main set changes to ease
review. Regarding the nft_rbtree fix, I'll provide a backport for
nf.git seperately.

Please apply, thanks!

[PATCH 1/5] netfilter: nft_rbtree: fix locking

[Patrick McHardy]

Fix a race condition and unnecessary locking:

* the root rb_node must only be accessed under the lock in nft_rbtree_lookup()
* the lock is not needed in lookup functions in netlink context

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

diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index 46214f2..2c75361 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -37,10 +37,11 @@ static bool nft_rbtree_lookup(const struct nft_set *set,
 {
 	const struct nft_rbtree *priv = nft_set_priv(set);
 	const struct nft_rbtree_elem *rbe, *interval = NULL;
-	const struct rb_node *parent = priv->root.rb_node;
+	const struct rb_node *parent;
 	int d;
 
 	spin_lock_bh(&nft_rbtree_lock);
+	parent = priv->root.rb_node;
 	while (parent != NULL) {
 		rbe = rb_entry(parent, struct nft_rbtree_elem, node);
 
@@ -158,7 +159,6 @@ static int nft_rbtree_get(const struct nft_set *set, struct nft_set_elem *elem)
 	struct nft_rbtree_elem *rbe;
 	int d;
 
-	spin_lock_bh(&nft_rbtree_lock);
 	while (parent != NULL) {
 		rbe = rb_entry(parent, struct nft_rbtree_elem, node);
 
@@ -173,11 +173,9 @@ static int nft_rbtree_get(const struct nft_set *set, struct nft_set_elem *elem)
 			    !(rbe->flags & NFT_SET_ELEM_INTERVAL_END))
 				nft_data_copy(&elem->data, rbe->data);
 			elem->flags = rbe->flags;
-			spin_unlock_bh(&nft_rbtree_lock);
 			return 0;
 		}
 	}
-	spin_unlock_bh(&nft_rbtree_lock);
 	return -ENOENT;
 }
 
-- 
2.1.0

[PATCH 2/5] netfilter: nf_tables: move struct net pointer to base chain

[Patrick McHardy]

The network namespace is only needed for base chains to get at the
gencursor. Also convert to possible_net_t.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h | 4 ++--
 net/netfilter/nf_tables_api.c     | 2 +-
 net/netfilter/nf_tables_core.c    | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index d756af5..ace67a5 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -449,7 +449,6 @@ enum nft_chain_flags {
  *
  *	@rules: list of rules in the chain
  *	@list: used internally
- *	@net: net namespace that this chain belongs to
  *	@table: table that this chain belongs to
  *	@handle: chain handle
  *	@use: number of jump references to this chain
@@ -460,7 +459,6 @@ enum nft_chain_flags {
 struct nft_chain {
 	struct list_head		rules;
 	struct list_head		list;
-	struct net			*net;
 	struct nft_table		*table;
 	u64				handle;
 	u32				use;
@@ -512,6 +510,7 @@ struct nft_stats {
  *	struct nft_base_chain - nf_tables base chain
  *
  *	@ops: netfilter hook ops
+ *	@pnet: net namespace that this chain belongs to
  *	@type: chain type
  *	@policy: default policy
  *	@stats: per-cpu chain stats
@@ -519,6 +518,7 @@ struct nft_stats {
  */
 struct nft_base_chain {
 	struct nf_hook_ops		ops[NFT_HOOK_OPS_MAX];
+	possible_net_t			pnet;
 	const struct nf_chain_type	*type;
 	u8				policy;
 	struct nft_stats __percpu	*stats;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index ea51833..334bf9c 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1349,6 +1349,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
 			rcu_assign_pointer(basechain->stats, stats);
 		}
 
+		write_pnet(&basechain->pnet, net);
 		basechain->type = type;
 		chain = &basechain->chain;
 
@@ -1376,7 +1377,6 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
 
 	INIT_LIST_HEAD(&chain->rules);
 	chain->handle = nf_tables_alloc_handle(table);
-	chain->net = net;
 	chain->table = table;
 	nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
 
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 77165bf..4c921a3 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -112,6 +112,7 @@ unsigned int
 nft_do_chain(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
 {
 	const struct nft_chain *chain = ops->priv, *basechain = chain;
+	const struct net *net = read_pnet(&nft_base_chain(basechain)->pnet);
 	const struct nft_rule *rule;
 	const struct nft_expr *expr, *last;
 	struct nft_data data[NFT_REG_MAX + 1];
@@ -123,7 +124,7 @@ nft_do_chain(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
 	 * Cache cursor to avoid problems in case that the cursor is updated
 	 * while traversing the ruleset.
 	 */
-	unsigned int gencursor = ACCESS_ONCE(chain->net->nft.gencursor);
+	unsigned int gencursor = ACCESS_ONCE(net->nft.gencursor);
 
 do_chain:
 	rulenum = 0;
-- 
2.1.0

Re: [PATCH 2/5] netfilter: nf_tables: move struct net pointer to base chain

[Pablo Neira Ayuso]

On Sat, Mar 21, 2015 at 03:19:15PM +0000, Patrick McHardy wrote:
> The network namespace is only needed for base chains to get at the
> gencursor. Also convert to possible_net_t.

Applied, thanks Patrick.

[PATCH 3/5] netfilter: nf_tables: reject NFT_SET_ELEM_INTERVAL_END flag for non-interval sets

[Patrick McHardy]

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

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 334bf9c..820f645 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3136,6 +3136,9 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 		elem.flags = ntohl(nla_get_be32(nla[NFTA_SET_ELEM_FLAGS]));
 		if (elem.flags & ~NFT_SET_ELEM_INTERVAL_END)
 			return -EINVAL;
+		if (!(set->flags & NFT_SET_INTERVAL) &&
+		    elem.flags & NFT_SET_ELEM_INTERVAL_END)
+			return -EINVAL;
 	}
 
 	if (set->flags & NFT_SET_MAP) {
-- 
2.1.0

[PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Patrick McHardy]

Following patches will add new private members, restore struct nft_hash
as preparation.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nft_hash.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 4585c57..80a6a6d 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -23,6 +23,10 @@
 /* We target a hash table size of 4, element hint is 75% of final size */
 #define NFT_HASH_ELEMENT_HINT 3
 
+struct nft_hash {
+	struct rhashtable		ht;
+};
+
 struct nft_hash_elem {
 	struct rhash_head		node;
 	struct nft_data			key;
@@ -35,10 +39,10 @@ static bool nft_hash_lookup(const struct nft_set *set,
 			    const struct nft_data *key,
 			    struct nft_data *data)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	const struct nft_hash_elem *he;
 
-	he = rhashtable_lookup_fast(priv, key, nft_hash_params);
+	he = rhashtable_lookup_fast(&priv->ht, key, nft_hash_params);
 	if (he && set->flags & NFT_SET_MAP)
 		nft_data_copy(data, he->data);
 
@@ -48,7 +52,7 @@ static bool nft_hash_lookup(const struct nft_set *set,
 static int nft_hash_insert(const struct nft_set *set,
 			   const struct nft_set_elem *elem)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	struct nft_hash_elem *he;
 	unsigned int size;
 	int err;
@@ -68,7 +72,7 @@ static int nft_hash_insert(const struct nft_set *set,
 	if (set->flags & NFT_SET_MAP)
 		nft_data_copy(he->data, &elem->data);
 
-	err = rhashtable_insert_fast(priv, &he->node, nft_hash_params);
+	err = rhashtable_insert_fast(&priv->ht, &he->node, nft_hash_params);
 	if (err)
 		kfree(he);
 
@@ -87,19 +91,19 @@ static void nft_hash_elem_destroy(const struct nft_set *set,
 static void nft_hash_remove(const struct nft_set *set,
 			    const struct nft_set_elem *elem)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 
-	rhashtable_remove_fast(priv, elem->cookie, nft_hash_params);
+	rhashtable_remove_fast(&priv->ht, elem->cookie, nft_hash_params);
 	synchronize_rcu();
 	kfree(elem->cookie);
 }
 
 static int nft_hash_get(const struct nft_set *set, struct nft_set_elem *elem)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	struct nft_hash_elem *he;
 
-	he = rhashtable_lookup_fast(priv, &elem->key, nft_hash_params);
+	he = rhashtable_lookup_fast(&priv->ht, &elem->key, nft_hash_params);
 	if (!he)
 		return -ENOENT;
 
@@ -114,13 +118,13 @@ static int nft_hash_get(const struct nft_set *set, struct nft_set_elem *elem)
 static void nft_hash_walk(const struct nft_ctx *ctx, const struct nft_set *set,
 			  struct nft_set_iter *iter)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	const struct nft_hash_elem *he;
 	struct rhashtable_iter hti;
 	struct nft_set_elem elem;
 	int err;
 
-	err = rhashtable_walk_init(priv, &hti);
+	err = rhashtable_walk_init(&priv->ht, &hti);
 	iter->err = err;
 	if (err)
 		return;
@@ -163,7 +167,7 @@ out:
 
 static unsigned int nft_hash_privsize(const struct nlattr * const nla[])
 {
-	return sizeof(struct rhashtable);
+	return sizeof(struct nft_hash);
 }
 
 static const struct rhashtable_params nft_hash_params = {
@@ -176,35 +180,35 @@ static int nft_hash_init(const struct nft_set *set,
 			 const struct nft_set_desc *desc,
 			 const struct nlattr * const tb[])
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	struct rhashtable_params params = nft_hash_params;
 
 	params.nelem_hint = desc->size ?: NFT_HASH_ELEMENT_HINT;
 	params.key_len = set->klen;
 
-	return rhashtable_init(priv, &params);
+	return rhashtable_init(&priv->ht, &params);
 }
 
 static void nft_hash_destroy(const struct nft_set *set)
 {
-	struct rhashtable *priv = nft_set_priv(set);
+	struct nft_hash *priv = nft_set_priv(set);
 	const struct bucket_table *tbl;
 	struct nft_hash_elem *he;
 	struct rhash_head *pos, *next;
 	unsigned int i;
 
 	/* Stop an eventual async resizing */
-	priv->being_destroyed = true;
-	mutex_lock(&priv->mutex);
+	priv->ht.being_destroyed = true;
+	mutex_lock(&priv->ht.mutex);
 
-	tbl = rht_dereference(priv->tbl, priv);
+	tbl = rht_dereference(priv->ht.tbl, &priv->ht);
 	for (i = 0; i < tbl->size; i++) {
 		rht_for_each_entry_safe(he, pos, next, tbl, i, node)
 			nft_hash_elem_destroy(set, he);
 	}
-	mutex_unlock(&priv->mutex);
+	mutex_unlock(&priv->ht.mutex);
 
-	rhashtable_destroy(priv);
+	rhashtable_destroy(&priv->ht);
 }
 
 static bool nft_hash_estimate(const struct nft_set_desc *desc, u32 features,
@@ -217,7 +221,7 @@ static bool nft_hash_estimate(const struct nft_set_desc *desc, u32 features,
 		esize += FIELD_SIZEOF(struct nft_hash_elem, data[0]);
 
 	if (desc->size) {
-		est->size = sizeof(struct rhashtable) +
+		est->size = sizeof(struct nft_hash) +
 			    roundup_pow_of_two(desc->size * 4 / 3) *
 			    sizeof(struct nft_hash_elem *) +
 			    desc->size * esize;
-- 
2.1.0

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Pablo Neira Ayuso]

On Sat, Mar 21, 2015 at 03:19:17PM +0000, Patrick McHardy wrote:
> Following patches will add new private members, restore struct nft_hash
> as preparation.

[2/2] netfilter: nft_hash: convert to use rhashtable callbacks
[1/2] rhashtable: provide len to obj_hashfn
[5/5] netfilter: nft_hash: indent rhashtable parameters
[4/5] netfilter: nft_hash: restore struct nft_hash

These patches don't apply anymore because of yesterday changes from
Thomas Graf:

http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/log/?qt=author&q=Thomas+Graf

:-(

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Patrick McHardy]

On 25.03, Pablo Neira Ayuso wrote:
> On Sat, Mar 21, 2015 at 03:19:17PM +0000, Patrick McHardy wrote:
> > Following patches will add new private members, restore struct nft_hash
> > as preparation.
> 
> [2/2] netfilter: nft_hash: convert to use rhashtable callbacks
> [1/2] rhashtable: provide len to obj_hashfn
> [5/5] netfilter: nft_hash: indent rhashtable parameters
> [4/5] netfilter: nft_hash: restore struct nft_hash
> 
> These patches don't apply anymore because of yesterday changes from
> Thomas Graf:
> 
> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/log/?qt=author&q=Thomas+Graf
> 
> :-(

Crap, they already sunk down in my tree :/

If you push out what you currently have, I'll rebase everything.

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Pablo Neira Ayuso]

On Wed, Mar 25, 2015 at 11:22:13AM +0000, Patrick McHardy wrote:
> On 25.03, Pablo Neira Ayuso wrote:
> > On Sat, Mar 21, 2015 at 03:19:17PM +0000, Patrick McHardy wrote:
> > > Following patches will add new private members, restore struct nft_hash
> > > as preparation.
> > 
> > [2/2] netfilter: nft_hash: convert to use rhashtable callbacks
> > [1/2] rhashtable: provide len to obj_hashfn
> > [5/5] netfilter: nft_hash: indent rhashtable parameters
> > [4/5] netfilter: nft_hash: restore struct nft_hash
> > 
> > These patches don't apply anymore because of yesterday changes from
> > Thomas Graf:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/log/?qt=author&q=Thomas+Graf
> > 
> > :-(
> 
> Crap, they already sunk down in my tree :/
> 
> If you push out what you currently have, I'll rebase everything.

Just pushed out.

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Patrick McHardy]

On 25.03, Pablo Neira Ayuso wrote:
> On Wed, Mar 25, 2015 at 11:22:13AM +0000, Patrick McHardy wrote:
> > On 25.03, Pablo Neira Ayuso wrote:
> > > On Sat, Mar 21, 2015 at 03:19:17PM +0000, Patrick McHardy wrote:
> > > > Following patches will add new private members, restore struct nft_hash
> > > > as preparation.
> > > 
> > > [2/2] netfilter: nft_hash: convert to use rhashtable callbacks
> > > [1/2] rhashtable: provide len to obj_hashfn
> > > [5/5] netfilter: nft_hash: indent rhashtable parameters
> > > [4/5] netfilter: nft_hash: restore struct nft_hash
> > > 
> > > These patches don't apply anymore because of yesterday changes from
> > > Thomas Graf:
> > > 
> > > http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/log/?qt=author&q=Thomas+Graf
> > > 
> > > :-(
> > 
> > Crap, they already sunk down in my tree :/
> > 
> > If you push out what you currently have, I'll rebase everything.
> 
> Just pushed out.

I'm getting really annoyed that people keep sending patches touching
this stuff without CCing either netfilter-devel or me personally.

This is just causing unnecessary work. The code in nft_hash is going
to get replaced anyways, so now I can rebase 30 patches for no gain
at all.

If you touch netfilter, CC the relevant lists and people please.
Thanks!

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Thomas Graf]

On 03/25/15 at 11:30am, Patrick McHardy wrote:
> I'm getting really annoyed that people keep sending patches touching
> this stuff without CCing either netfilter-devel or me personally.
> 
> This is just causing unnecessary work. The code in nft_hash is going
> to get replaced anyways, so now I can rebase 30 patches for no gain
> at all.
> 
> If you touch netfilter, CC the relevant lists and people please.
> Thanks!

Sorry about that Patrick. Not copying you on the nft_hash change
was a mistake. I apologize. I was only aware of your following
patchset which seemed compatible with my changes.

"[PATCH 0/2] netfilter: preparatory patches for set extensions"

If your larger patchset resolves the RCU lockdep splash in nft_hash
then we can also just revert 6b6f302ce ("rhashtable: Add
rhashtable_free_and_destroy()") or I can rebase it on top of your
series.

Re: [PATCH 4/5] netfilter: nft_hash: restore struct nft_hash

[Patrick McHardy]

On 25.03, Thomas Graf wrote:
> On 03/25/15 at 11:30am, Patrick McHardy wrote:
> > I'm getting really annoyed that people keep sending patches touching
> > this stuff without CCing either netfilter-devel or me personally.
> > 
> > This is just causing unnecessary work. The code in nft_hash is going
> > to get replaced anyways, so now I can rebase 30 patches for no gain
> > at all.
> > 
> > If you touch netfilter, CC the relevant lists and people please.
> > Thanks!
> 
> Sorry about that Patrick. Not copying you on the nft_hash change
> was a mistake. I apologize. I was only aware of your following
> patchset which seemed compatible with my changes.

Its usually not a big deal if you forget, but it has happened
too many times recently with rhashtable, so I was getting a little
upset. Anyways, apology accepted :)

> "[PATCH 0/2] netfilter: preparatory patches for set extensions"
> 
> If your larger patchset resolves the RCU lockdep splash in nft_hash
> then we can also just revert 6b6f302ce ("rhashtable: Add
> rhashtable_free_and_destroy()") or I can rebase it on top of your
> series.

I'd like to prevent further delays, so I've already started merging
everything on top of that change. Thanks anyway.

[PATCH 5/5] netfilter: nft_hash: indent rhashtable parameters

[Patrick McHardy]

Improve readability by indenting the parameter initialization.

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

diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 80a6a6d..e815940 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -171,9 +171,9 @@ static unsigned int nft_hash_privsize(const struct nlattr * const nla[])
 }
 
 static const struct rhashtable_params nft_hash_params = {
-	.head_offset = offsetof(struct nft_hash_elem, node),
-	.key_offset = offsetof(struct nft_hash_elem, key),
-	.hashfn = jhash,
+	.head_offset	= offsetof(struct nft_hash_elem, node),
+	.key_offset	= offsetof(struct nft_hash_elem, key),
+	.hashfn		= jhash,
 };
 
 static int nft_hash_init(const struct nft_set *set,
@@ -184,7 +184,7 @@ static int nft_hash_init(const struct nft_set *set,
 	struct rhashtable_params params = nft_hash_params;
 
 	params.nelem_hint = desc->size ?: NFT_HASH_ELEMENT_HINT;
-	params.key_len = set->klen;
+	params.key_len	  = set->klen;
 
 	return rhashtable_init(&priv->ht, &params);
 }
-- 
2.1.0

Re: [PATCH 0/5] netfilter: nf_tables: assorted patches

[Pablo Neira Ayuso]

On Sat, Mar 21, 2015 at 03:19:13PM +0000, Patrick McHardy wrote:
> Hi Pablo,
> 
> following are five more or less assorted patches for nf_tables:
> 
> * fix for a minor locking issue in nft_rbtree
> * move the struct net pointer from the chain to the base chain
> * check for invalid NFT_SET_ELEM_INTERVAL_END centrally
> * restruct struct nft_hash as preparation for following patches
> * minor indentation cleanup for nft_hash

Applied 1/5 and 3/5.

2/5 needs possible_net_t which is not yet in nf-next. Also 4/5 and 5/5
need changes from net-next.

I'll apply these once I get that new type into nf-next. Will send a
nf-next pull request asap, sorry.

> I'm sending these patches seperately from the main set changes to ease
> review. Regarding the nft_rbtree fix, I'll provide a backport for
> nf.git seperately.

No problem, I'll pass the rbtree fix to -stable.

Thanks Patrick.