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

[PATCH v3 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
 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(-)

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);
-- 
2.39.3 (Apple Git-145)

[PATCH v3 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
 v3: Do not call nft_trans_destroy() on table updates in nf_tables_commit()

 include/net/netfilter/nf_tables.h |  3 ++
 net/netfilter/nf_tables_api.c     | 56 ++++++++++++++++++++++---------
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 144dc469ebf8..43c747bd3f80 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1684,10 +1684,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 85088297dd0d..62a2a1798052 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -188,6 +188,17 @@ static struct nlattr *nft_userdata_dup(const struct nlattr *udata, gfp_t gfp)
 	return kmemdup(udata, nla_total_size(nla_len(udata)), gfp);
 }
 
+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 void __nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set,
 				 bool bind)
 {
@@ -1210,16 +1221,16 @@ 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) &&
@@ -1240,6 +1251,14 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
 	if (trans == NULL)
 		return -ENOMEM;
 
+	if (udata) {
+		nft_trans_table_udata(trans) = nft_userdata_dup(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;
@@ -1271,6 +1290,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;
 }
@@ -9378,6 +9399,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);
@@ -10140,19 +10164,18 @@ 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));
+				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
 			} else {
 				nft_clear(net, trans->ctx.table);
+				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
+				nft_trans_destroy(trans);
 			}
-			nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
-			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_DELTABLE:
 		case NFT_MSG_DESTROYTABLE:
@@ -10430,6 +10453,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 v3 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
> handling for it to support table comment updates.

One generic API question below.  Pablo, please look at this too.

>  		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));
> +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE

AFAICS this means that if the table as udata attached, and userspace
makes an update request without a UDATA netlink attribute, we will
delete the existing udata.

Is that right?

My question is, should we instead leave the existing udata as-is and not
support removal, only replace?

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

[Pablo Neira Ayuso]

On Tue, Mar 12, 2024 at 01:27:58PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
> > handling for it to support table comment updates.
> 
> One generic API question below.  Pablo, please look at this too.
> 
> >  		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));
> > +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE
> 
> AFAICS this means that if the table as udata attached, and userspace
> makes an update request without a UDATA netlink attribute, we will
> delete the existing udata.
> 
> Is that right?
> 
> My question is, should we instead leave the existing udata as-is and not
> support removal, only replace?

I would leave it in place too if no _USERDATA is specified.

One more question is if the memcmp() with old and new udata makes
sense considering two consecutive requests for _USERDATA update in one
batch.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > AFAICS this means that if the table as udata attached, and userspace
> > makes an update request without a UDATA netlink attribute, we will
> > delete the existing udata.
> > 
> > Is that right?
> > 
> > My question is, should we instead leave the existing udata as-is and not
> > support removal, only replace?
> 
> I would leave it in place too if no _USERDATA is specified.
> 
> One more question is if the memcmp() with old and new udata makes
> sense considering two consecutive requests for _USERDATA update in one
> batch.

Great point, any second udata change request in the same batch must fail.

We learned this the hard way with flag updates :(

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

[Quan Tian]

On Tue, Mar 12, 2024 at 02:01:34PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > AFAICS this means that if the table as udata attached, and userspace
> > > makes an update request without a UDATA netlink attribute, we will
> > > delete the existing udata.
> > > 
> > > Is that right?
> > > 
> > > My question is, should we instead leave the existing udata as-is and not
> > > support removal, only replace?
> > 
> > I would leave it in place too if no _USERDATA is specified.
> > 

Sure, I will change it in the proposed way.

> > One more question is if the memcmp() with old and new udata makes
> > sense considering two consecutive requests for _USERDATA update in one
> > batch.
> 
> Great point, any second udata change request in the same batch must fail.
> 
> We learned this the hard way with flag updates :(

Is it the same as two consectutive requests for chain name update and
chain stats update? 

In nf_tables_commit():
The 1st trans swaps old udata with 1st new udata;
The 2nd trans swaps 1st new udata with 2nd new udata.

In nft_commit_release():
The 1st trans frees old udata;
The 2nd trans frees 1st new udata.

So multiple udata requests in a batch could work?

Thanks,
Quan

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

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> In nf_tables_commit():
> The 1st trans swaps old udata with 1st new udata;
> The 2nd trans swaps 1st new udata with 2nd new udata.
> 
> In nft_commit_release():
> The 1st trans frees old udata;
> The 2nd trans frees 1st new udata.
> 
> So multiple udata requests in a batch could work?

Yes, it could work indeed but we got bitten by
subtle bugs with back-to-back updates.

If there is a simple way to detect and reject
this then I believe its better to disallow it.

Unless you come up with a use-case where such back-to-back
udate updates make sense of course.

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

[Pablo Neira Ayuso]

On Tue, Mar 12, 2024 at 03:33:00PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > In nf_tables_commit():
> > The 1st trans swaps old udata with 1st new udata;
> > The 2nd trans swaps 1st new udata with 2nd new udata.
> > 
> > In nft_commit_release():
> > The 1st trans frees old udata;
> > The 2nd trans frees 1st new udata.
> > 
> > So multiple udata requests in a batch could work?
> 
> Yes, it could work indeed but we got bitten by
> subtle bugs with back-to-back updates.

yes, we have seen subtle bugs recently. As for the table flags, the
dormant flag has been particularly a problem, it is tricky one because
it registers hooks from preparation step (which might fail) but it
cannot register hooks because abort path might need undo things, and
re-register the hooks could lead to failures from a later path which
does not admit failures. For the dormant flag, another possibility
would be to handle this from the core, so there is no need to register
and unregister hooks, instead simply set them as inactive.

The dormant flag use case is rather corner case, but hardware offload
will require sooner or later a mode to run in _hardware mode only_
(currently it is both hardware and software for nftables) and
considering the hardware offload API has been designed for packet
classifiers from the late 90s (that is, very strictly express your
policy in a linear ruleset) that means dropping packets early is fine,
but wanted traffic gets evaluated in a linear list.

> If there is a simple way to detect and reject
> this then I believe its better to disallow it.

That requires to iterate over the list of transaction, or add some
kind of flag to reject this.

> Unless you come up with a use-case where such back-to-back
> udate updates make sense of course.

I don't have a use-case for this table userdata myself, this is
currently only used to store comments by userspace, why someone would
be willing to update such comment associated to a table, I don't know.

I would like to know if there are plans to submit similar patches for
other objects. As for sets, this needs to be careful because userdata
contains the set description.

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

[Quan Tian]

On Tue, Mar 12, 2024 at 11:23:56PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 12, 2024 at 03:33:00PM +0100, Florian Westphal wrote:
> > Quan Tian <tianquan23@gmail.com> wrote:
> > > In nf_tables_commit():
> > > The 1st trans swaps old udata with 1st new udata;
> > > The 2nd trans swaps 1st new udata with 2nd new udata.
> > > 
> > > In nft_commit_release():
> > > The 1st trans frees old udata;
> > > The 2nd trans frees 1st new udata.
> > > 
> > > So multiple udata requests in a batch could work?
> > 
> > Yes, it could work indeed but we got bitten by
> > subtle bugs with back-to-back updates.
> 
> yes, we have seen subtle bugs recently. As for the table flags, the
> dormant flag has been particularly a problem, it is tricky one because
> it registers hooks from preparation step (which might fail) but it
> cannot register hooks because abort path might need undo things, and
> re-register the hooks could lead to failures from a later path which
> does not admit failures. For the dormant flag, another possibility
> would be to handle this from the core, so there is no need to register
> and unregister hooks, instead simply set them as inactive.
> 
> The dormant flag use case is rather corner case, but hardware offload
> will require sooner or later a mode to run in _hardware mode only_
> (currently it is both hardware and software for nftables) and
> considering the hardware offload API has been designed for packet
> classifiers from the late 90s (that is, very strictly express your
> policy in a linear ruleset) that means dropping packets early is fine,
> but wanted traffic gets evaluated in a linear list.
> 
> > If there is a simple way to detect and reject
> > this then I believe its better to disallow it.
> 
> That requires to iterate over the list of transaction, or add some
> kind of flag to reject this.
> 
> > Unless you come up with a use-case where such back-to-back
> > udate updates make sense of course.
> 
> I don't have a use-case for this table userdata myself, this is
> currently only used to store comments by userspace, why someone would
> be willing to update such comment associated to a table, I don't know.

There was a use-case in kube-proxy that we wanted to use comment to
store version/compatibility information so we could know whether it
has to recreate the whole table when upgrading to a new version due to
incompatible chain/rule changes (e.g. a chain's hook and priority is
changed). The reason why we wanted to avoid recreating the whole table
when it doesn't have to is because deleting the table would also
destory the dynamic sets in the table, losing some data.

Another minor reason is that the comments could be used to store
human-readable explanation for the objects. And they may be change when
the objects' functions change. It would be great if they could be
updated via the "add" operation, otherwise "delete+add" would always be
needed to keep comments up-to-date.

However, I don't know a use-case for back-to-back comment updates, I 
will check which one is more complex and risky: rejecting it or
supporting it.

> I would like to know if there are plans to submit similar patches for
> other objects. As for sets, this needs to be careful because userdata
> contains the set description.

As explained above, I think there is some value in supporting comment
update for other objects, especially when the objects contain dynamic
data. But I understand there are risks like you mentioned and it would
need to be more careful than tables. I would volunteer to take a try
for other objects, starting with objects similiar to tables first, if
it doesn’t sound bad to you. Please let me know if you feel it's not
worth.

Thanks,
Quan

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

[Pablo Neira Ayuso]

On Wed, Mar 13, 2024 at 09:35:10AM +0800, Quan Tian wrote:
> On Tue, Mar 12, 2024 at 11:23:56PM +0100, Pablo Neira Ayuso wrote:
[...]
> > I don't have a use-case for this table userdata myself, this is
> > currently only used to store comments by userspace, why someone would
> > be willing to update such comment associated to a table, I don't know.
> 
> There was a use-case in kube-proxy that we wanted to use comment to
> store version/compatibility information so we could know whether it
> has to recreate the whole table when upgrading to a new version due to
> incompatible chain/rule changes (e.g. a chain's hook and priority is
> changed). The reason why we wanted to avoid recreating the whole table
> when it doesn't have to is because deleting the table would also
> destory the dynamic sets in the table, losing some data.

There is a generation number which gets bumped for each ruleset
update which is currently global.

Would having such generation ID per table help or you need more
flexibility in what needs to be stored in the userdata area?

> Another minor reason is that the comments could be used to store
> human-readable explanation for the objects. And they may be change when
> the objects' functions change. It would be great if they could be
> updated via the "add" operation, otherwise "delete+add" would always be
> needed to keep comments up-to-date.
> 
> However, I don't know a use-case for back-to-back comment updates, I
> will check which one is more complex and risky: rejecting it or
> supporting it.

We have these back-to-back update everywhere. The original idea was to
allow for robots to place all commands in a batch and send it to the
kernel, if the robot adds objects and then, while the same batch is
being collected, another update on such object happens, including one
that cancels the object that was just added, this is accepted since
this nonsense is valid according to the transactional model.

In this transactional model, batch processing also continues on
errors, rather than stopping on the first error, error unwinding is
special in that sense because objects remain in place and are just
marked are deleted. This allows for getting all errors to userspace at
once.

It is harder than classic netdev interfaces in the kernel.

Florian has concerns on this transactional model as too hard because
of the recent bug reports. If the direction is to tigthen this, then
I think all should be revised, not just reject this userdata update
while everything else still allows for back-to-back updates.

This is of course not related to your patch.

> > I would like to know if there are plans to submit similar patches for
> > other objects. As for sets, this needs to be careful because userdata
> > contains the set description.
> 
> As explained above, I think there is some value in supporting comment
> update for other objects, especially when the objects contain dynamic
> data. But I understand there are risks like you mentioned and it would
> need to be more careful than tables. I would volunteer to take a try
> for other objects, starting with objects similiar to tables first, if
> it doesn’t sound bad to you. Please let me know if you feel it's not
> worth.

I am not saying it is not worth, and I am trying to understand your
requirements in case there is a chance to provide an alternative path
to using _USERDATA for this.

Thanks for explaining.

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

[Quan Tian]

On Wed, Mar 13, 2024 at 10:40:05AM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 13, 2024 at 09:35:10AM +0800, Quan Tian wrote:
> > On Tue, Mar 12, 2024 at 11:23:56PM +0100, Pablo Neira Ayuso wrote:
> [...]
> > > I don't have a use-case for this table userdata myself, this is
> > > currently only used to store comments by userspace, why someone would
> > > be willing to update such comment associated to a table, I don't know.
> > 
> > There was a use-case in kube-proxy that we wanted to use comment to
> > store version/compatibility information so we could know whether it
> > has to recreate the whole table when upgrading to a new version due to
> > incompatible chain/rule changes (e.g. a chain's hook and priority is
> > changed). The reason why we wanted to avoid recreating the whole table
> > when it doesn't have to is because deleting the table would also
> > destory the dynamic sets in the table, losing some data.
> 
> There is a generation number which gets bumped for each ruleset
> update which is currently global.
> 
> Would having such generation ID per table help or you need more
> flexibility in what needs to be stored in the userdata area?

Auto-increased generation ID may not meet the above requirement as the
table could also be frequently updated by configuration changes at
runtime, not only after the application upgrades. An example scenario
could be: say we have a loadbalancer application based on nftables:

* LB v0.1.0 installs a collection of nftable rules;
* LB v0.1.1 makes some changes compatible with v0.1.0. In upgrade case,
  it doesn't need to recreate the whole table if the existing rules
  were installed by version >= 0.1.0;
* LB v0.2.0 makes some changes incompatible with v0.1.x (e.g. some
  chains' priorities are changed), it needs to recreate the whole table
  when upgrading from v0.1.x to it;
* LB v0.2.1 makes some changes compatible with v0.2.0, it doesn't need
  to recreate the table when upgrading from v0.2.0 to it but needs to
  recreate it when upgrading from v0.1.x to it.

With the support for comment updates, we can store, get, and update
such version information in comment, and use it to determine when it's
necessary to recreate the table.

>> If there is a simple way to detect and reject
>> this then I believe its better to disallow it.
>
> That requires to iterate over the list of transaction, or add some
> kind of flag to reject this.

I tried to detect back-to-back userdata comment updates, but found it's
more complex than I thought. A flag may not work because it can only
tell whether the 1st update touched comment and we don't know whether
the 2nd update is the same as the 1st one, unless we iterate over the
list of transaction, which looks a bit overkill? It seems simpler if we
just allow it and accept the last userdata.

>> My question is, should we instead leave the existing udata as-is and not
>> support removal, only replace?
>
>I would leave it in place too if no _USERDATA is specified.

I got a question when trying to implementing this. If the update request
specifies USERDATA but its length is 0, do we treat it like not
specified, or remove the existing userdata? Asking because I see
nf_tables_newrule() treats 0 length in the same way as unspecified and
doesn't initialize a nft_userdata. It makes sense for create, but I'm
not sure if it's right to do it in the same way for update. 

And if we want to treat it as "remove comment", we can't simply add a
single pointer of nft_userdata to nft_trans_table to achieve it, because
there would be 3 cases: leave it in place, remove it, update it.

Thanks,
Quan

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

[Pablo Neira Ayuso]

On Tue, Mar 12, 2024 at 10:26:17PM +0800, Quan Tian wrote:
> On Tue, Mar 12, 2024 at 02:01:34PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > AFAICS this means that if the table as udata attached, and userspace
> > > > makes an update request without a UDATA netlink attribute, we will
> > > > delete the existing udata.
> > > > 
> > > > Is that right?
> > > > 
> > > > My question is, should we instead leave the existing udata as-is and not
> > > > support removal, only replace?
> > > 
> > > I would leave it in place too if no _USERDATA is specified.
> > > 
> 
> Sure, I will change it in the proposed way.
> 
> > > One more question is if the memcmp() with old and new udata makes
> > > sense considering two consecutive requests for _USERDATA update in one
> > > batch.
> > 
> > Great point, any second udata change request in the same batch must fail.
> > 
> > We learned this the hard way with flag updates :(
> 
> Is it the same as two consectutive requests for chain name update and
> chain stats update? 
> 
> In nf_tables_commit():
> The 1st trans swaps old udata with 1st new udata;
> The 2nd trans swaps 1st new udata with 2nd new udata.
>
> In nft_commit_release():
> The 1st trans frees old udata;
> The 2nd trans frees 1st new udata.

And abort path simply releases the transaction objects, since udata
was left intact in place.

> So multiple udata requests in a batch could work?

Yes, if rcu is used correctly, that should work fine. But memcmp()
needs to be removed.

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

[Pablo Neira Ayuso]

On Mon, Mar 11, 2024 at 10:14:54PM +0800, Quan Tian wrote:
> The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
> handling for it to support table comment updates.

dump path is lockless:

        if (table->udata) {
                if (nla_put(skb, NFTA_TABLE_USERDATA, table->udlen, table->udata))
                        goto nla_put_failure;
        }

there are two things to update at the same time here, table->udata and
table->udlen.

This needs to be reworked fully if updates are required.

nft_userdata {
        const u8        *data;
        u16             len;
}

then, update struct nft_table to have:

        struct nft_userdata __rcu *user;

then, from dump path, rcu_dereference struct nft_userdata pointer.

BTW, does swap() ensure rcu semantics?

> Signed-off-by: Quan Tian <tianquan23@gmail.com>
> ---
>  v2: Change to store userdata in struct nlattr * to ensure atomical update
>  v3: Do not call nft_trans_destroy() on table updates in nf_tables_commit()
> 
>  include/net/netfilter/nf_tables.h |  3 ++
>  net/netfilter/nf_tables_api.c     | 56 ++++++++++++++++++++++---------
>  2 files changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 144dc469ebf8..43c747bd3f80 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1684,10 +1684,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 85088297dd0d..62a2a1798052 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -188,6 +188,17 @@ static struct nlattr *nft_userdata_dup(const struct nlattr *udata, gfp_t gfp)
>  	return kmemdup(udata, nla_total_size(nla_len(udata)), gfp);
>  }
>  
> +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 void __nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set,
>  				 bool bind)
>  {
> @@ -1210,16 +1221,16 @@ 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) &&
> @@ -1240,6 +1251,14 @@ static int nf_tables_updtable(struct nft_ctx *ctx)
>  	if (trans == NULL)
>  		return -ENOMEM;
>  
> +	if (udata) {
> +		nft_trans_table_udata(trans) = nft_userdata_dup(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;
> @@ -1271,6 +1290,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;
>  }
> @@ -9378,6 +9399,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);
> @@ -10140,19 +10164,18 @@ 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));
> +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
>  			} else {
>  				nft_clear(net, trans->ctx.table);
> +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> +				nft_trans_destroy(trans);
>  			}
> -			nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> -			nft_trans_destroy(trans);
>  			break;
>  		case NFT_MSG_DELTABLE:
>  		case NFT_MSG_DESTROYTABLE:
> @@ -10430,6 +10453,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 v3 nf-next 2/2] netfilter: nf_tables: support updating userdata for nft_table

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Mar 11, 2024 at 10:14:54PM +0800, Quan Tian wrote:
> > The NFTA_TABLE_USERDATA attribute was ignored on updates. The patch adds
> > handling for it to support table comment updates.
> 
> dump path is lockless:
> 
>         if (table->udata) {
>                 if (nla_put(skb, NFTA_TABLE_USERDATA, table->udlen, table->udata))
>                         goto nla_put_failure;
>         }
> 
> there are two things to update at the same time here, table->udata and
> table->udlen.
> 
> This needs to be reworked fully if updates are required.

See first patch in the series, it makes this a single pointer,
but you are right...

> then, update struct nft_table to have:
> 
>         struct nft_userdata __rcu *user;

.. this needs an __rcu annotation.  I'll respond
to patch 1 too.

> BTW, does swap() ensure rcu semantics?

No, this needs to use rcu_replace_pointer() and manual update
of the old one stored in the transaction update.

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

[Florian Westphal]

Quan Tian <tianquan23@gmail.com> wrote:
> -				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> +				swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
> +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);

I missed this in my review, as Pablo pointed out you can't use swap()
here, because table->udata is rcu protected.

Something like this should work as replacement:

nft_trans_table_udata(trans) = rcu_replace_pointer(trans->ctx.table->udata,
                                                   nft_trans_table_udata(trans),
						   lockdep_commit_lock_is_held(trans->ctx.net));

This will swap and ensure all stores are visible.

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

[Quan Tian]

On Tue, Mar 12, 2024 at 03:10:46PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@gmail.com> wrote:
> > -				trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> > +				swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
> > +				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
> 
> I missed this in my review, as Pablo pointed out you can't use swap()
> here, because table->udata is rcu protected.
> 
> Something like this should work as replacement:
> 
> nft_trans_table_udata(trans) = rcu_replace_pointer(trans->ctx.table->udata,
>                                                    nft_trans_table_udata(trans),
> 						   lockdep_commit_lock_is_held(trans->ctx.net));
> 
> This will swap and ensure all stores are visible.

Thank you Pablo and Florian for pointing it out. I will fix it following
your suggestions.

Thanks,
Quan

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

[Florian Westphal]

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.

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

[Pablo Neira Ayuso]

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.

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

[Florian Westphal]

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.

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

[Quan Tian]

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[];
};