diff mbox series

[nf,v2,1/8] netfilter: nf_tables: Don't allocate nft_rule_dump_ctx

Message ID 20230928165244.7168-2-phil@nwl.cc
State Changes Requested, archived
Headers show
Series Introduce locking for reset requests | expand

Commit Message

Phil Sutter Sept. 28, 2023, 4:52 p.m. UTC 
Eliminate the direct use of netlink_callback::args when dumping rules by
casting nft_rule_dump_ctx over netlink_callback::ctx as suggested in
the struct's comment.

The value for 's_idx' has to be stored inside nft_rule_dump_ctx now and
make it hold the 'reset' boolean as well.

Note how this patch removes the zeroing of netlink_callback::args[1-5] -
none of the rule dump callbacks seem to make use of them.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Fix description: 'idx' is not moved into the struct
---
 net/netfilter/nf_tables_api.c | 81 ++++++++++++++---------------------
 1 file changed, 32 insertions(+), 49 deletions(-)

Comments

Pablo Neira Ayuso Sept. 28, 2023, 6:49 p.m. UTC | #1 
On Thu, Sep 28, 2023 at 06:52:37PM +0200, Phil Sutter wrote:
[...]

This whole chunk below looks like a cleanup to remove one indentation
level? Please add an initial patch for this.

>  static int nf_tables_dump_rules_start(struct netlink_callback *cb)
>  {
> +	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
>  	const struct nlattr * const *nla = cb->data;
> -	struct nft_rule_dump_ctx *ctx = NULL;
>  
> -	if (nla[NFTA_RULE_TABLE] || nla[NFTA_RULE_CHAIN]) {
> -		ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC);
> -		if (!ctx)
> -			return -ENOMEM;
> +	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
>  
> -		if (nla[NFTA_RULE_TABLE]) {
> -			ctx->table = nla_strdup(nla[NFTA_RULE_TABLE],
> -							GFP_ATOMIC);
> -			if (!ctx->table) {
> -				kfree(ctx);
> -				return -ENOMEM;
> -			}
> -		}
> -		if (nla[NFTA_RULE_CHAIN]) {
> -			ctx->chain = nla_strdup(nla[NFTA_RULE_CHAIN],
> -						GFP_ATOMIC);
> -			if (!ctx->chain) {
> -				kfree(ctx->table);
> -				kfree(ctx);
> -				return -ENOMEM;
> -			}
> +	if (nla[NFTA_RULE_TABLE]) {
> +		ctx->table = nla_strdup(nla[NFTA_RULE_TABLE], GFP_ATOMIC);
> +		if (!ctx->table)
> +			return -ENOMEM;
> +	}
> +	if (nla[NFTA_RULE_CHAIN]) {
> +		ctx->chain = nla_strdup(nla[NFTA_RULE_CHAIN], GFP_ATOMIC);
> +		if (!ctx->chain) {
> +			kfree(ctx->table);
> +			return -ENOMEM;
>  		}
>  	}
> +	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
> +		ctx->reset = true;
>  
> -	cb->data = ctx;
>  	return 0;
>  }
>  
>  static int nf_tables_dump_rules_done(struct netlink_callback *cb)
>  {
> -	struct nft_rule_dump_ctx *ctx = cb->data;
> +	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
>  
> -	if (ctx) {
> -		kfree(ctx->table);
> -		kfree(ctx->chain);
> -		kfree(ctx);
> -	}
> +	kfree(ctx->table);
> +	kfree(ctx->chain);
>  	return 0;
>  }
>  
> -- 
> 2.41.0
>
Florian Westphal Sept. 28, 2023, 7 p.m. UTC | #2 
Phil Sutter <phil@nwl.cc> wrote:
> Eliminate the direct use of netlink_callback::args when dumping rules by
> casting nft_rule_dump_ctx over netlink_callback::ctx as suggested in
> the struct's comment.
> 
> The value for 's_idx' has to be stored inside nft_rule_dump_ctx now and
> make it hold the 'reset' boolean as well.
> 
> Note how this patch removes the zeroing of netlink_callback::args[1-5] -
> none of the rule dump callbacks seem to make use of them.

Do you think we can fix the reset race in -next instead of -nf?

If yes, you could detach preparation patches like this one and
split the series in several batches.
Phil Sutter Sept. 29, 2023, 10:13 a.m. UTC | #3 
On Thu, Sep 28, 2023 at 09:00:44PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Eliminate the direct use of netlink_callback::args when dumping rules by
> > casting nft_rule_dump_ctx over netlink_callback::ctx as suggested in
> > the struct's comment.
> > 
> > The value for 's_idx' has to be stored inside nft_rule_dump_ctx now and
> > make it hold the 'reset' boolean as well.
> > 
> > Note how this patch removes the zeroing of netlink_callback::args[1-5] -
> > none of the rule dump callbacks seem to make use of them.
> 
> Do you think we can fix the reset race in -next instead of -nf?
> 
> If yes, you could detach preparation patches like this one and
> split the series in several batches.

Yes, I noticed this series is no longer the "add some spinlock to
prevent races" it was in the beginning.

TBH, I chose nf mostly because nf-next lacked a commit I needed. But
it's there now, so v3 will address nf-next.

Thanks, Phil
Phil Sutter Sept. 29, 2023, 10:15 a.m. UTC | #4 
On Thu, Sep 28, 2023 at 08:49:33PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 28, 2023 at 06:52:37PM +0200, Phil Sutter wrote:
> [...]
> 
> This whole chunk below looks like a cleanup to remove one indentation
> level? Please add an initial patch for this.

Actually, it changes nft_rule_dump_ctx from being allocated to being
cast over the general purpose part in netlink_callback. But I like your
suggestion, it will reduce this patch to the relevant bits.

Thanks, Phil
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4356189360fb8..511508407867d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3441,20 +3441,21 @@  static void audit_log_rule_reset(const struct nft_table *table,
 }
 
 struct nft_rule_dump_ctx {
+	unsigned int s_idx;
 	char *table;
 	char *chain;
+	bool reset;
 };
 
 static int __nf_tables_dump_rules(struct sk_buff *skb,
 				  unsigned int *idx,
 				  struct netlink_callback *cb,
 				  const struct nft_table *table,
-				  const struct nft_chain *chain,
-				  bool reset)
+				  const struct nft_chain *chain)
 {
+	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
 	struct net *net = sock_net(skb->sk);
 	const struct nft_rule *rule, *prule;
-	unsigned int s_idx = cb->args[0];
 	unsigned int entries = 0;
 	int ret = 0;
 	u64 handle;
@@ -3463,12 +3464,9 @@  static int __nf_tables_dump_rules(struct sk_buff *skb,
 	list_for_each_entry_rcu(rule, &chain->rules, list) {
 		if (!nft_is_active(net, rule))
 			goto cont_skip;
-		if (*idx < s_idx)
+		if (*idx < ctx->s_idx)
 			goto cont;
-		if (*idx > s_idx) {
-			memset(&cb->args[1], 0,
-					sizeof(cb->args) - sizeof(cb->args[0]));
-		}
+
 		if (prule)
 			handle = prule->handle;
 		else
@@ -3479,7 +3477,7 @@  static int __nf_tables_dump_rules(struct sk_buff *skb,
 					NFT_MSG_NEWRULE,
 					NLM_F_MULTI | NLM_F_APPEND,
 					table->family,
-					table, chain, rule, handle, reset) < 0) {
+					table, chain, rule, handle, ctx->reset) < 0) {
 			ret = 1;
 			break;
 		}
@@ -3491,7 +3489,7 @@  static int __nf_tables_dump_rules(struct sk_buff *skb,
 		(*idx)++;
 	}
 
-	if (reset && entries)
+	if (ctx->reset && entries)
 		audit_log_rule_reset(table, cb->seq, entries);
 
 	return ret;
@@ -3501,17 +3499,13 @@  static int nf_tables_dump_rules(struct sk_buff *skb,
 				struct netlink_callback *cb)
 {
 	const struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh);
-	const struct nft_rule_dump_ctx *ctx = cb->data;
+	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
 	struct nft_table *table;
 	const struct nft_chain *chain;
 	unsigned int idx = 0;
 	struct net *net = sock_net(skb->sk);
 	int family = nfmsg->nfgen_family;
 	struct nftables_pernet *nft_net;
-	bool reset = false;
-
-	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
-		reset = true;
 
 	rcu_read_lock();
 	nft_net = nft_pernet(net);
@@ -3521,10 +3515,10 @@  static int nf_tables_dump_rules(struct sk_buff *skb,
 		if (family != NFPROTO_UNSPEC && family != table->family)
 			continue;
 
-		if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0)
+		if (ctx->table && strcmp(ctx->table, table->name) != 0)
 			continue;
 
-		if (ctx && ctx->table && ctx->chain) {
+		if (ctx->table && ctx->chain) {
 			struct rhlist_head *list, *tmp;
 
 			list = rhltable_lookup(&table->chains_ht, ctx->chain,
@@ -3536,7 +3530,7 @@  static int nf_tables_dump_rules(struct sk_buff *skb,
 				if (!nft_is_active(net, chain))
 					continue;
 				__nf_tables_dump_rules(skb, &idx,
-						       cb, table, chain, reset);
+						       cb, table, chain);
 				break;
 			}
 			goto done;
@@ -3544,62 +3538,51 @@  static int nf_tables_dump_rules(struct sk_buff *skb,
 
 		list_for_each_entry_rcu(chain, &table->chains, list) {
 			if (__nf_tables_dump_rules(skb, &idx,
-						   cb, table, chain, reset))
+						   cb, table, chain))
 				goto done;
 		}
 
-		if (ctx && ctx->table)
+		if (ctx->table)
 			break;
 	}
 done:
 	rcu_read_unlock();
 
-	cb->args[0] = idx;
+	ctx->s_idx = idx;
 	return skb->len;
 }
 
 static int nf_tables_dump_rules_start(struct netlink_callback *cb)
 {
+	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
 	const struct nlattr * const *nla = cb->data;
-	struct nft_rule_dump_ctx *ctx = NULL;
 
-	if (nla[NFTA_RULE_TABLE] || nla[NFTA_RULE_CHAIN]) {
-		ctx = kzalloc(sizeof(*ctx), GFP_ATOMIC);
-		if (!ctx)
-			return -ENOMEM;
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
 
-		if (nla[NFTA_RULE_TABLE]) {
-			ctx->table = nla_strdup(nla[NFTA_RULE_TABLE],
-							GFP_ATOMIC);
-			if (!ctx->table) {
-				kfree(ctx);
-				return -ENOMEM;
-			}
-		}
-		if (nla[NFTA_RULE_CHAIN]) {
-			ctx->chain = nla_strdup(nla[NFTA_RULE_CHAIN],
-						GFP_ATOMIC);
-			if (!ctx->chain) {
-				kfree(ctx->table);
-				kfree(ctx);
-				return -ENOMEM;
-			}
+	if (nla[NFTA_RULE_TABLE]) {
+		ctx->table = nla_strdup(nla[NFTA_RULE_TABLE], GFP_ATOMIC);
+		if (!ctx->table)
+			return -ENOMEM;
+	}
+	if (nla[NFTA_RULE_CHAIN]) {
+		ctx->chain = nla_strdup(nla[NFTA_RULE_CHAIN], GFP_ATOMIC);
+		if (!ctx->chain) {
+			kfree(ctx->table);
+			return -ENOMEM;
 		}
 	}
+	if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET)
+		ctx->reset = true;
 
-	cb->data = ctx;
 	return 0;
 }
 
 static int nf_tables_dump_rules_done(struct netlink_callback *cb)
 {
-	struct nft_rule_dump_ctx *ctx = cb->data;
+	struct nft_rule_dump_ctx *ctx = (void *)cb->ctx;
 
-	if (ctx) {
-		kfree(ctx->table);
-		kfree(ctx->chain);
-		kfree(ctx);
-	}
+	kfree(ctx->table);
+	kfree(ctx->chain);
 	return 0;
 }