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

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

[Quan Tian]

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
---
 include/net/netfilter/nf_tables.h | 4 +---
 net/netfilter/nf_tables_api.c     | 9 +++++----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 510244cc0f8f..3944eb969172 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 7e938c7397dd..de7efb8c8089 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -983,7 +983,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;
 	}
 
@@ -1386,11 +1387,11 @@ 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 = kmemdup(nla[NFTA_TABLE_USERDATA],
+				       nla_total_size(nla_len(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);
-- 
2.39.3 (Apple Git-145)

[PATCH v2 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

[Quan Tian]

The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
handling for it to support table comment updates.

Signed-off-by: Quan Tian <tianquan23@gmail.com>
---
v2: Change to store userdata in struct nlattr * to ensure atomical update
---
 include/net/netfilter/nf_tables.h |  3 ++
 net/netfilter/nf_tables_api.c     | 52 ++++++++++++++++++++++---------
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3944eb969172..db324b4d4651 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1678,10 +1678,13 @@ struct nft_trans_chain {
 
 struct nft_trans_table {
 	bool				update;
+	struct nlattr			*udata;
 };
 
 #define nft_trans_table_update(trans)	\
 	(((struct nft_trans_table *)trans->data)->update)
+#define nft_trans_table_udata(trans)	\
+	(((struct nft_trans_table *)trans->data)->udata)
 
 struct nft_trans_elem {
 	struct nft_set			*set;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index de7efb8c8089..b89a2734ab20 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1199,20 +1199,31 @@ static void nf_tables_table_disable(struct net *net, struct nft_table *table)
 #define __NFT_TABLE_F_UPDATE		(__NFT_TABLE_F_WAS_DORMANT | \
 					 __NFT_TABLE_F_WAS_AWAKEN)
 
+static bool nft_userdata_is_same(const struct nlattr *a, const struct nlattr *b)
+{
+	if (!a && !b)
+		return true;
+	if (!a || !b)
+		return false;
+	if (nla_len(a) != nla_len(b))
+		return false;
+	return !memcmp(nla_data(a), nla_data(b), nla_len(a));
+}
+
 static int nf_tables_updtable(struct nft_ctx *ctx)
 {
 	struct nft_trans *trans;
 	u32 flags;
+	const struct nlattr *udata = ctx->nla[NFTA_TABLE_USERDATA];
 	int ret;
 
-	if (!ctx->nla[NFTA_TABLE_FLAGS])
-		return 0;
-
-	flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
-	if (flags & ~NFT_TABLE_F_MASK)
-		return -EOPNOTSUPP;
+	if (ctx->nla[NFTA_TABLE_FLAGS]) {
+		flags = ntohl(nla_get_be32(ctx->nla[NFTA_TABLE_FLAGS]));
+		if (flags & ~NFT_TABLE_F_MASK)
+			return -EOPNOTSUPP;
+	}
 
-	if (flags == ctx->table->flags)
+	if (flags == ctx->table->flags && nft_userdata_is_same(udata, ctx->table->udata))
 		return 0;
 
 	if ((nft_table_has_owner(ctx->table) &&
@@ -1230,6 +1241,15 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (trans == NULL)
 		return -ENOMEM;
 
+	if (udata) {
+		nft_trans_table_udata(trans) = kmemdup(udata, nla_total_size(nla_len(udata)),
+						       GFP_KERNEL_ACCOUNT);
+		if (!nft_trans_table_udata(trans)) {
+			ret = -ENOMEM;
+			goto err_table_udata;
+		}
+	}
+
 	if ((flags & NFT_TABLE_F_DORMANT) &&
 	    !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
 		ctx->table->flags |= NFT_TABLE_F_DORMANT;
@@ -1254,6 +1274,8 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 
 err_register_hooks:
 	ctx->table->flags |= NFT_TABLE_F_DORMANT;
+	kfree(nft_trans_table_udata(trans));
+err_table_udata:
 	nft_trans_destroy(trans);
 	return ret;
 }
@@ -9367,6 +9389,9 @@ static void nft_obj_commit_update(struct nft_trans *trans)
 static void nft_commit_release(struct nft_trans *trans)
 {
 	switch (trans->msg_type) {
+	case NFT_MSG_NEWTABLE:
+		kfree(nft_trans_table_udata(trans));
+		break;
 	case NFT_MSG_DELTABLE:
 	case NFT_MSG_DESTROYTABLE:
 		nf_tables_table_destroy(&trans->ctx);
@@ -10129,14 +10154,12 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
-				if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
-					nft_trans_destroy(trans);
-					break;
+				if (trans->ctx.table->flags & __NFT_TABLE_F_UPDATE) {
+					if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
+						nf_tables_table_disable(net, trans->ctx.table);
+					trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
 				}
-				if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
-					nf_tables_table_disable(net, trans->ctx.table);
-
-				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
+				swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
 			} else {
 				nft_clear(net, trans->ctx.table);
 			}
@@ -10419,6 +10442,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
+				kfree(nft_trans_table_udata(trans));
 				if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
 					nft_trans_destroy(trans);
 					break;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH v2 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> @@ -10129,14 +10154,12 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  		switch (trans->msg_type) {
>  		case NFT_MSG_NEWTABLE:
>  			if (nft_trans_table_update(trans)) {
> -				if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
> -					nft_trans_destroy(trans);
> -					break;
> +				if (trans->ctx.table->flags & __NFT_TABLE_F_UPDATE) {
> +					if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> +						nf_tables_table_disable(net, trans->ctx.table);
> +					trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
>  				}
> -				if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> -					nf_tables_table_disable(net, trans->ctx.table);
> -
> -				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> +				swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
>  			} else {
>  				nft_clear(net, trans->ctx.table);
>  			}

There is a call to nft_trans_destroy() below here.
You could add a "break" after the swap() to avoid it.

Otherwise kmemleak should report old udata being lost
on update.

Re: [PATCH v2 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

[Quan Tian]

Hi Florian,

Thanks a lot for your reviews.

On Sun, Mar 10, 2024 at 06:47:54PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > @@ -10129,14 +10154,12 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
> >  		switch (trans->msg_type) {
> >  		case NFT_MSG_NEWTABLE:
> >  			if (nft_trans_table_update(trans)) {
> > -				if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
> > -					nft_trans_destroy(trans);
> > -					break;
> > +				if (trans->ctx.table->flags & __NFT_TABLE_F_UPDATE) {
> > +					if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> > +						nf_tables_table_disable(net, trans->ctx.table);
> > +					trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> >  				}
> > -				if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> > -					nf_tables_table_disable(net, trans->ctx.table);
> > -
> > -				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> > +				swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
> >  			} else {
> >  				nft_clear(net, trans->ctx.table);
> >  			}
> 
> There is a call to nft_trans_destroy() below here.
> You could add a "break" after the swap() to avoid it.
> 
> Otherwise kmemleak should report old udata being lost
> on update.

Thanks for pointing it out. kmemleak indeed reported the leak.
I found "break" after swap() would skip sending the table update event,
so I changed to execute different code paths for the two branches in v3.
Please let me know if it makes sense to you.

Thanks,
Quan

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

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> 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

Looks good, one minor nit below.

>  	if (nla[NFTA_TABLE_USERDATA]) {
> -		table->udata = nla_memdup(nla[NFTA_TABLE_USERDATA], GFP_KERNEL_ACCOUNT);
> +		table->udata = kmemdup(nla[NFTA_TABLE_USERDATA],
> +				       nla_total_size(nla_len(nla[NFTA_TABLE_USERDATA])),
> +				       GFP_KERNEL_ACCOUNT);

I think its correct but it might make sense to add a small helper for
this kmemdup so we don't need to copypaste in case this should get
extended to e.g. chain udata update support.

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

[Quan Tian]

On Mon, Mar 11, 2024 at 12:09:23AM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > 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
> 
> Looks good, one minor nit below.
> 
> >  	if (nla[NFTA_TABLE_USERDATA]) {
> > -		table->udata = nla_memdup(nla[NFTA_TABLE_USERDATA], GFP_KERNEL_ACCOUNT);
> > +		table->udata = kmemdup(nla[NFTA_TABLE_USERDATA],
> > +				       nla_total_size(nla_len(nla[NFTA_TABLE_USERDATA])),
> > +				       GFP_KERNEL_ACCOUNT);
> 
> I think its correct but it might make sense to add a small helper for
> this kmemdup so we don't need to copypaste in case this should get
> extended to e.g. chain udata update support.

Extracted a function as suggested in v3, and also used it when
duplicating the userdata when preparing trans for table updates.
After this is merged, I could extend comment update support for other
objects in the same fashion.

Thanks,
Quan