[PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[Fernando Fernandez Mancera]

This patch adds the infrastructure needed for the stateful object update
support.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 include/net/netfilter/nf_tables.h |  6 +++
 net/netfilter/nf_tables_api.c     | 71 ++++++++++++++++++++++++++++---
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index dc301e3d6739..dc4e32040ea9 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1123,6 +1123,9 @@ struct nft_object_ops {
 	int				(*dump)(struct sk_buff *skb,
 						struct nft_object *obj,
 						bool reset);
+	int				(*update)(const struct nft_ctx *ctx,
+						  const struct nlattr *const tb[],
+						  struct nft_object *obj);
 	const struct nft_object_type	*type;
 };
 
@@ -1405,10 +1408,13 @@ struct nft_trans_elem {
 
 struct nft_trans_obj {
 	struct nft_object		*obj;
+	bool				update;
 };
 
 #define nft_trans_obj(trans)	\
 	(((struct nft_trans_obj *)trans->data)->obj)
+#define nft_trans_obj_update(trans)	\
+	(((struct nft_trans_obj *)trans->data)->update)
 
 struct nft_trans_flowtable {
 	struct nft_flowtable		*flowtable;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index fe3b7b0c6c66..5ab4b0636213 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5122,6 +5122,46 @@ nft_obj_type_get(struct net *net, u32 objtype)
 	return ERR_PTR(-ENOENT);
 }
 
+static int nf_tables_updobj(const struct nft_ctx *ctx,
+			    const struct nft_object_type *type,
+			    const struct nlattr *attr,
+			    struct nft_object *obj)
+{
+	struct nft_trans *trans;
+	struct nlattr **tb;
+	int err = -ENOMEM;
+
+	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
+				sizeof(struct nft_trans_obj));
+	if (!trans)
+		return -ENOMEM;
+
+	tb = kcalloc(type->maxattr + 1, sizeof(*tb), GFP_KERNEL);
+	if (!tb)
+		goto err;
+
+	if (attr) {
+		err = nla_parse_nested_deprecated(tb, type->maxattr, attr,
+						  type->policy, NULL);
+		if (err < 0)
+			goto err;	
+	}
+
+	err = obj->ops->update(ctx, (const struct nlattr * const *)tb, obj);
+	if (err < 0)
+		goto err;
+
+	nft_trans_obj_update(trans) = true;
+	list_add_tail(&trans->list, &ctx->net->nft.commit_list);
+
+	kfree(tb);
+	return 0;
+
+err:
+	nft_trans_destroy(trans);
+	return err;
+}
+
 static int nf_tables_newobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,
 			    const struct nlattr * const nla[],
@@ -5161,7 +5201,13 @@ static int nf_tables_newobj(struct net *net, struct sock *nlsk,
 			NL_SET_BAD_ATTR(extack, nla[NFTA_OBJ_NAME]);
 			return -EEXIST;
 		}
-		return 0;
+		if (nlh->nlmsg_flags & NLM_F_REPLACE)
+			return -EOPNOTSUPP;
+
+		type = nft_obj_type_get(net, objtype);
+		nft_ctx_init(&ctx, net, skb, nlh, family, table, NULL, nla);
+
+		return nf_tables_updobj(&ctx, type, nla[NFTA_OBJ_DATA], obj);
 	}
 
 	nft_ctx_init(&ctx, net, skb, nlh, family, table, NULL, nla);
@@ -6786,10 +6832,17 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 			te->set->ndeact--;
 			break;
 		case NFT_MSG_NEWOBJ:
-			nft_clear(net, nft_trans_obj(trans));
-			nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
-					     NFT_MSG_NEWOBJ);
-			nft_trans_destroy(trans);
+			if (nft_trans_obj_update(trans)) {
+				nf_tables_obj_notify(&trans->ctx,
+						     nft_trans_obj(trans),
+						     NFT_MSG_NEWOBJ);
+			} else {
+				nft_clear(net, nft_trans_obj(trans));
+				nf_tables_obj_notify(&trans->ctx,
+						     nft_trans_obj(trans),
+						     NFT_MSG_NEWOBJ);
+				nft_trans_destroy(trans);
+			}
 			break;
 		case NFT_MSG_DELOBJ:
 			nft_obj_del(nft_trans_obj(trans));
@@ -6936,8 +6989,12 @@ static int __nf_tables_abort(struct net *net)
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWOBJ:
-			trans->ctx.table->use--;
-			nft_obj_del(nft_trans_obj(trans));
+			if (nft_trans_obj_update(trans)) {
+				nft_trans_destroy(trans);
+			} else {
+				trans->ctx.table->use--;
+				nft_obj_del(nft_trans_obj(trans));
+			}
 			break;
 		case NFT_MSG_DELOBJ:
 			trans->ctx.table->use++;
-- 
2.20.1

[PATCH 2/2 nf-next] netfilter: nft_quota: add quota object update support

[Fernando Fernandez Mancera]

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 net/netfilter/nft_quota.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
index c8745d454bf8..ad95cac61e2d 100644
--- a/net/netfilter/nft_quota.c
+++ b/net/netfilter/nft_quota.c
@@ -105,6 +105,44 @@ static int nft_quota_obj_init(const struct nft_ctx *ctx,
 	return nft_quota_do_init(tb, priv);
 }
 
+static int nft_quota_do_update(const struct nlattr * const tb[],
+			       struct nft_quota * priv)
+{
+	unsigned long flags;
+	u64 quota;
+
+	flags = priv->flags;
+	quota = priv->quota;
+
+	if (tb[NFTA_QUOTA_BYTES]) {
+		quota = be64_to_cpu(nla_get_be64(tb[NFTA_QUOTA_BYTES]));
+		if (quota > S64_MAX)
+			return -EOVERFLOW;
+	}
+
+	if (tb[NFTA_QUOTA_FLAGS]) {
+		flags = ntohl(nla_get_be32(tb[NFTA_QUOTA_FLAGS]));
+		if (flags & ~NFT_QUOTA_F_INV)
+			return -EINVAL;
+		if (flags & ~NFT_QUOTA_F_DEPLETED)
+			return -EOPNOTSUPP;
+	}
+
+	priv->quota = quota;
+	priv->flags = flags;
+
+	return 0;
+}
+
+static int nft_quota_obj_update(const struct nft_ctx *ctx,
+				const struct nlattr * const tb[],
+				struct nft_object *obj)
+{
+	struct nft_quota *priv = nft_obj_data(obj);
+
+	return nft_quota_do_update(tb, priv);
+}
+
 static int nft_quota_do_dump(struct sk_buff *skb, struct nft_quota *priv,
 			     bool reset)
 {
@@ -155,6 +193,7 @@ static const struct nft_object_ops nft_quota_obj_ops = {
 	.init		= nft_quota_obj_init,
 	.eval		= nft_quota_obj_eval,
 	.dump		= nft_quota_obj_dump,
+	.update		= nft_quota_obj_update,
 };
 
 static struct nft_object_type nft_quota_obj_type __read_mostly = {
-- 
2.20.1

Re: [PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[Florian Westphal]

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> This patch adds the infrastructure needed for the stateful object update
> support.
> 
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  include/net/netfilter/nf_tables.h |  6 +++
>  net/netfilter/nf_tables_api.c     | 71 ++++++++++++++++++++++++++++---
>  2 files changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index dc301e3d6739..dc4e32040ea9 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1123,6 +1123,9 @@ struct nft_object_ops {
>  	int				(*dump)(struct sk_buff *skb,
>  						struct nft_object *obj,
>  						bool reset);
> +	int				(*update)(const struct nft_ctx *ctx,
> +						  const struct nlattr *const tb[],
> +						  struct nft_object *obj);

maybe adda 'bool commit' argument here.

> +	err = obj->ops->update(ctx, (const struct nlattr * const *)tb, obj);

Then, set it to 'false' here.
You would have to keep 'tb' allocated and place it on the 'trans'
object.

> +	nft_trans_obj_update(trans) = true;

	nft_trans_obj_update_tb(trans) = tb;

> -			nft_clear(net, nft_trans_obj(trans));
> -			nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
> -					     NFT_MSG_NEWOBJ);
> -			nft_trans_destroy(trans);
> +			if (nft_trans_obj_update(trans)) {

				nft_trans_obj(trans)->ops->update(&trans->ctx,
					      nft_trans_obj_update_tb(trans),
					      nft_trans_obj(trans),
					      true);

				kfree(nft_trans_obj_update_tb(trans));


Because otherwise we will update objects while we're not yet sure that
we can process/handle the entire batch.

I think we should, if possible, only update once we've made it to
the commit phase.

Re: [PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[Fernando Fernandez Mancera]

On 8/21/19 12:09 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
>> This patch adds the infrastructure needed for the stateful object update
>> support.
>>
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>>  include/net/netfilter/nf_tables.h |  6 +++
>>  net/netfilter/nf_tables_api.c     | 71 ++++++++++++++++++++++++++++---
>>  2 files changed, 70 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
>> index dc301e3d6739..dc4e32040ea9 100644
>> --- a/include/net/netfilter/nf_tables.h
>> +++ b/include/net/netfilter/nf_tables.h
>> @@ -1123,6 +1123,9 @@ struct nft_object_ops {
>>  	int				(*dump)(struct sk_buff *skb,
>>  						struct nft_object *obj,
>>  						bool reset);
>> +	int				(*update)(const struct nft_ctx *ctx,
>> +						  const struct nlattr *const tb[],
>> +						  struct nft_object *obj);
> 
> maybe adda 'bool commit' argument here.
> 

How is that argument going to be used? If 'commit' is false we should
just check that values are fine but not update them?

>> +	err = obj->ops->update(ctx, (const struct nlattr * const *)tb, obj);
> 
> Then, set it to 'false' here.
> You would have to keep 'tb' allocated and place it on the 'trans'
> object.
> 
Yes, I agree on updating the object in the commit phase. But I am not
sure about how I should place it on 'trans'. Any hints? Thanks :-)

I am also writing some userspace shell tests.

>> +	nft_trans_obj_update(trans) = true;
> 
> 	nft_trans_obj_update_tb(trans) = tb;
> 
>> -			nft_clear(net, nft_trans_obj(trans));
>> -			nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
>> -					     NFT_MSG_NEWOBJ);
>> -			nft_trans_destroy(trans);
>> +			if (nft_trans_obj_update(trans)) {
> 
> 				nft_trans_obj(trans)->ops->update(&trans->ctx,
> 					      nft_trans_obj_update_tb(trans),
> 					      nft_trans_obj(trans),
> 					      true);
> 
> 				kfree(nft_trans_obj_update_tb(trans));
> 
> 
> Because otherwise we will update objects while we're not yet sure that
> we can process/handle the entire batch.
> 
> I think we should, if possible, only update once we've made it to
> the commit phase.
> 

Re: [PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[Florian Westphal]

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> How is that argument going to be used? If 'commit' is false we should
> just check that values are fine but not update them?

Yes, thats the idea.

> Yes, I agree on updating the object in the commit phase. But I am not
> sure about how I should place it on 'trans'. Any hints? Thanks :-)

Can you place a pointer to the tb array on the trans object?

Another possibility is to have ->update return a kmalloced blob
that contains ready-to-use binary data, so depending on the 'bool
commit' the update hook would expect either tb[] (for validation)
or a backend-maintained struct with the to-update values.

In the quota case it would be a struct containing the u64 values.

> I am also writing some userspace shell tests.

Thats good, thanks Fernando!

Re: [PATCH 1/2 nf-next] netfilter: nf_tables: Introduce stateful object update operation

[Fernando Fernandez Mancera]

On 8/21/19 1:11 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
>> How is that argument going to be used? If 'commit' is false we should
>> just check that values are fine but not update them?
> 
> Yes, thats the idea.
> 
>> Yes, I agree on updating the object in the commit phase. But I am not
>> sure about how I should place it on 'trans'. Any hints? Thanks :-)
> 
> Can you place a pointer to the tb array on the trans object?
> 

I prefer this option. So we can place a pointer to the tb array on the
trans object and also the pointer to the existing object. This way it
should be easy to do ->update in the commit phase.

Thanks!

> Another possibility is to have ->update return a kmalloced blob
> that contains ready-to-use binary data, so depending on the 'bool
> commit' the update hook would expect either tb[] (for validation)
> or a backend-maintained struct with the to-update values.
> 
> In the quota case it would be a struct containing the u64 values.
> 
>> I am also writing some userspace shell tests.
> 
> Thats good, thanks Fernando!
>