diff mbox series

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

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

Commit Message

Quan Tian March 10, 2024, 5:28 p.m. UTC 
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(-)

Comments

Florian Westphal March 10, 2024, 5:47 p.m. UTC | #1 
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.
Quan Tian March 11, 2024, 2:12 p.m. UTC | #2 
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
diff mbox series

Patch

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;