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

[PATCH nf-next] 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>
---
 include/net/netfilter/nf_tables.h |  6 ++++
 net/netfilter/nf_tables_api.c     | 53 +++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 510244cc0f8f..4406c056f52f 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1680,10 +1680,16 @@ struct nft_trans_chain {
 
 struct nft_trans_table {
 	bool				update;
+	u16				udlen;
+	u8				*udata;
 };
 
 #define nft_trans_table_update(trans)	\
 	(((struct nft_trans_table *)trans->data)->update)
+#define nft_trans_table_udlen(trans)	\
+	(((struct nft_trans_table *)trans->data)->udlen)
+#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 7e938c7397dd..9243f4b02895 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1198,20 +1198,32 @@ 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 *nla, u8 *udata, u16 udlen)
+{
+	if (!nla && !udata)
+		return true;
+	if (!nla || !udata)
+		return false;
+	if (nla_len(nla) != udlen)
+		return false;
+	return !nla_memcmp(nla, udata, udlen);
+}
+
 static int nf_tables_updtable(struct nft_ctx *ctx)
 {
 	struct nft_trans *trans;
 	u32 flags;
 	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(ctx->nla[NFTA_TABLE_USERDATA], ctx->table->udata,
+				 ctx->table->udlen))
 		return 0;
 
 	if ((nft_table_has_owner(ctx->table) &&
@@ -1229,6 +1241,16 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (trans == NULL)
 		return -ENOMEM;
 
+	if (ctx->nla[NFTA_TABLE_USERDATA]) {
+		nft_trans_table_udata(trans) = nla_memdup(ctx->nla[NFTA_TABLE_USERDATA],
+							  GFP_KERNEL_ACCOUNT);
+		if (!nft_trans_table_udata(trans)) {
+			ret = -ENOMEM;
+			goto err_table_udata;
+		}
+		nft_trans_table_udlen(trans) = nla_len(ctx->nla[NFTA_TABLE_USERDATA]);
+	}
+
 	if ((flags & NFT_TABLE_F_DORMANT) &&
 	    !(ctx->table->flags & NFT_TABLE_F_DORMANT)) {
 		ctx->table->flags |= NFT_TABLE_F_DORMANT;
@@ -1253,6 +1275,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;
 }
@@ -10128,14 +10152,14 @@ 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;
+				kfree(trans->ctx.table->udata);
+				trans->ctx.table->udata = nft_trans_table_udata(trans);
+				trans->ctx.table->udlen = nft_trans_table_udlen(trans);
 			} else {
 				nft_clear(net, trans->ctx.table);
 			}
@@ -10418,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 nf-next] netfilter: nf_tables: support updating userdata for nft_table

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> -				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;
> +				kfree(trans->ctx.table->udata);

This kfree() needs to happen in nft_commit_release().

Otherwise netlink dumps (which run without a lock) results in
use-after-free.

Second issue is that ->udata and ->udlen would have to be
updated atomically so that async dump doesn't observe old/new
pointer with a larger udlen.

I suggest a preparation patch that replaces

   u16  udlen;
   u8   *udata;

in struct nft_table with

	struct nlattr *udata;

so its enough to swap() ctx.table->udata with nft_trans_table_udata(),
then kfree() the old udata from nft_commit_release().

At _release time we can be sure no other cpu is referencing the old
udata.