diff mbox series

[v3,nf-next,1/2] netfilter: nf_tables: use struct nlattr * to store userdata for nft_table

Message ID 20240311141454.31537-1-tianquan23@gmail.com
State New
Headers show
Series [v3,nf-next,1/2] netfilter: nf_tables: use struct nlattr * to store userdata for nft_table | expand

Commit Message

Quan Tian March 11, 2024, 2:14 p.m. UTC 
To prepare for the support for table comment updates, the patch changes
to store userdata in struct nlattr *, which can be updated atomically on
updates.

Signed-off-by: Quan Tian <tianquan23@gmail.com>
---
 v2: Change to store userdata in struct nlattr * to ensure atomical update
 v3: Extract a helper function to duplicate userdata

 include/net/netfilter/nf_tables.h |  4 +---
 net/netfilter/nf_tables_api.c     | 13 +++++++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

Comments

Florian Westphal March 12, 2024, 2:05 p.m. UTC | #1 
Quan Tian <tianquan23@gmail.com> wrote:
>  	u32				nlpid;
>  	char				*name;
> -	u16				udlen;
> -	u8				*udata;
> +	struct nlattr			*udata;

I missed this detail.  As Pablo pointed out this pointer
now needs a __rcu annotation.

And this needs something like:

struct nlattr *udata = rcu_dereference(table->udata);

if (udata) {
 	if (nla_put(skb, NFTA_TABLE_USERDATA, nla_len(udata), nla_data(udata)))

> +		if (nla_put(skb, NFTA_TABLE_USERDATA, nla_len(table->udata),
> +			    nla_data(table->udata)))

... because this version can observe different table->udata for
nla_len() and nla_data() calls if the swap() has the "right" / "wrong"
timing.
Pablo Neira Ayuso March 12, 2024, 2:17 p.m. UTC | #2 
On Tue, Mar 12, 2024 at 03:05:35PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> >  	u32				nlpid;
> >  	char				*name;
> > -	u16				udlen;
> > -	u8				*udata;
> > +	struct nlattr			*udata;

May I suggest to use our own data structure, instead of using nlattr?
It is just a bit misleading to the reader.

But maybe I need to get used to this and that's all, your call.

Thanks.
Florian Westphal March 12, 2024, 2:34 p.m. UTC | #3 
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +	struct nlattr			*udata;
> 
> May I suggest to use our own data structure, instead of using nlattr?
> It is just a bit misleading to the reader.
> 
> But maybe I need to get used to this and that's all, your call.

I have no preference.  I thought reusing nlattr was simpler
because you can just kmemdup the nlattr+header.

I'll leave it up to patch author, no strong opinion either.
Quan Tian March 12, 2024, 3:03 p.m. UTC | #4 
On Tue, Mar 12, 2024 at 03:34:44PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > +	struct nlattr			*udata;
> > 
> > May I suggest to use our own data structure, instead of using nlattr?
> > It is just a bit misleading to the reader.
> > 
> > But maybe I need to get used to this and that's all, your call.
> 
> I have no preference.  I thought reusing nlattr was simpler
> because you can just kmemdup the nlattr+header.
> 
> I'll leave it up to patch author, no strong opinion either.

I found an existing struct nft_userdata defined in nf_tables.h and used
by nft_rule. Perhaps I could reuse it for nft_table? Its struct is as
below:

struct nft_userdata {
	u8			len;
	unsigned char		data[];
};
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index e27c28b612e4..144dc469ebf8 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1248,7 +1248,6 @@  static inline void nft_use_inc_restore(u32 *use)
  *	@genmask: generation mask
  *	@nlpid: netlink port ID
  *	@name: name of the table
- *	@udlen: length of the user data
  *	@udata: user data
  *	@validate_state: internal, set when transaction adds jumps
  */
@@ -1267,8 +1266,7 @@  struct nft_table {
 					genmask:2;
 	u32				nlpid;
 	char				*name;
-	u16				udlen;
-	u8				*udata;
+	struct nlattr			*udata;
 	u8				validate_state;
 };
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 224e5fb6a916..85088297dd0d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -183,6 +183,11 @@  static void nft_trans_destroy(struct nft_trans *trans)
 	kfree(trans);
 }
 
+static struct nlattr *nft_userdata_dup(const struct nlattr *udata, gfp_t gfp)
+{
+	return kmemdup(udata, nla_total_size(nla_len(udata)), gfp);
+}
+
 static void __nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set,
 				 bool bind)
 {
@@ -983,7 +988,8 @@  static int nf_tables_fill_table_info(struct sk_buff *skb, struct net *net,
 		goto nla_put_failure;
 
 	if (table->udata) {
-		if (nla_put(skb, NFTA_TABLE_USERDATA, table->udlen, table->udata))
+		if (nla_put(skb, NFTA_TABLE_USERDATA, nla_len(table->udata),
+			    nla_data(table->udata)))
 			goto nla_put_failure;
 	}
 
@@ -1398,11 +1404,10 @@  static int nf_tables_newtable(struct sk_buff *skb, const struct nfnl_info *info,
 		goto err_strdup;
 
 	if (nla[NFTA_TABLE_USERDATA]) {
-		table->udata = nla_memdup(nla[NFTA_TABLE_USERDATA], GFP_KERNEL_ACCOUNT);
+		table->udata = nft_userdata_dup(nla[NFTA_TABLE_USERDATA],
+						GFP_KERNEL_ACCOUNT);
 		if (table->udata == NULL)
 			goto err_table_udata;
-
-		table->udlen = nla_len(nla[NFTA_TABLE_USERDATA]);
 	}
 
 	err = rhltable_init(&table->chains_ht, &nft_chain_ht_params);