netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
@ 2019-08-22 16:48 Fernando Fernandez Mancera
  2019-08-22 16:48 ` [PATCH 2/2 nf-next v2] netfilter: nft_quota: add quota object update support Fernando Fernandez Mancera
  2019-08-23 12:41 ` [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Pablo Neira Ayuso
  0 siblings, 2 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-22 16:48 UTC (permalink / raw)
  To: netfilter-devel; +Cc: 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 | 10 ++++
 net/netfilter/nf_tables_api.c     | 90 ++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index dc301e3d6739..3b6e300b21af 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1123,6 +1123,10 @@ 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,
+						  bool commit);
 	const struct nft_object_type	*type;
 };
 
@@ -1405,10 +1409,16 @@ struct nft_trans_elem {
 
 struct nft_trans_obj {
 	struct nft_object		*obj;
+	struct nlattr			**tb;
+	bool				update;
 };
 
 #define nft_trans_obj(trans)	\
 	(((struct nft_trans_obj *)trans->data)->obj)
+#define nft_trans_obj_tb(trans) \
+	(((struct nft_trans_obj *)trans->data)->tb)
+#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..810ef50275dc 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5122,6 +5122,49 @@ 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, false);
+	if (err < 0)
+		goto err;
+
+	nft_trans_obj(trans) = obj;
+	nft_trans_obj_update(trans) = true;
+	nft_trans_obj_tb(trans) = tb;
+	list_add_tail(&trans->list, &ctx->net->nft.commit_list);
+
+	return 0;
+
+err:
+	nft_trans_destroy(trans);
+	kfree(tb);
+	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 +5204,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);
@@ -6422,6 +6471,20 @@ static void nft_chain_commit_update(struct nft_trans *trans)
 	}
 }
 
+static void nft_obj_commit_update(struct nft_trans *trans)
+{
+	struct nft_object *obj;
+	struct nlattr **tb;
+
+	obj = nft_trans_obj(trans);
+	tb = nft_trans_obj_tb(trans);
+
+	obj->ops->update(&trans->ctx, (const struct nlattr * const *)tb,
+			 obj, true);
+
+	kfree(tb);
+}
+
 static void nft_commit_release(struct nft_trans *trans)
 {
 	switch (trans->msg_type) {
@@ -6786,10 +6849,18 @@ 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)) {
+				nft_obj_commit_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 +7007,13 @@ 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)) {
+				kfree(nft_trans_obj_tb(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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2 nf-next v2] netfilter: nft_quota: add quota object update support
  2019-08-22 16:48 [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Fernando Fernandez Mancera
@ 2019-08-22 16:48 ` Fernando Fernandez Mancera
  2019-08-23 12:41 ` [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Pablo Neira Ayuso
  1 sibling, 0 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-22 16:48 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Fernando Fernandez Mancera

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

diff --git a/net/netfilter/nft_quota.c b/net/netfilter/nft_quota.c
index c8745d454bf8..2afea3f50a51 100644
--- a/net/netfilter/nft_quota.c
+++ b/net/netfilter/nft_quota.c
@@ -105,6 +105,47 @@ 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, bool commit)
+{
+	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;
+	}
+
+	if (commit) {
+		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,
+				bool commit)
+{
+	struct nft_quota *priv = nft_obj_data(obj);
+
+	return nft_quota_do_update(tb, priv, commit);
+}
+
 static int nft_quota_do_dump(struct sk_buff *skb, struct nft_quota *priv,
 			     bool reset)
 {
@@ -155,6 +196,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-22 16:48 [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Fernando Fernandez Mancera
  2019-08-22 16:48 ` [PATCH 2/2 nf-next v2] netfilter: nft_quota: add quota object update support Fernando Fernandez Mancera
@ 2019-08-23 12:41 ` Pablo Neira Ayuso
  2019-08-23 12:42   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-23 12:41 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>  
>  struct nft_trans_obj {
>  	struct nft_object		*obj;
> +	struct nlattr			**tb;

Instead of annotatint tb[] on the object, you can probably add here:

union {
        struct quota {
                uint64_t                consumed;
                uint64_t                quota;
      } quota;
};

So the initial update annotates the values in the transaction.

I guess you will need two new indirections? Something like
prepare_update() and update().

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-23 12:41 ` [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Pablo Neira Ayuso
@ 2019-08-23 12:42   ` Pablo Neira Ayuso
  2019-08-23 18:05     ` Fernando Fernandez Mancera
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-23 12:42 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
> > @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
> >  
> >  struct nft_trans_obj {
> >  	struct nft_object		*obj;
> > +	struct nlattr			**tb;
> 
> Instead of annotatint tb[] on the object, you can probably add here:
> 
> union {
>         struct quota {
>                 uint64_t                consumed;
>                 uint64_t                quota;
>       } quota;
> };
> 
> So the initial update annotates the values in the transaction.
> 
> I guess you will need two new indirections? Something like
> prepare_update() and update().

Or you have a single update() and pass enum nft_trans_phase as
parameter, so this only needs one single indirection.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-23 12:42   ` Pablo Neira Ayuso
@ 2019-08-23 18:05     ` Fernando Fernandez Mancera
  2019-08-23 18:28       ` Fernando Fernandez Mancera
  0 siblings, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-23 18:05 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel



On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>>>  
>>>  struct nft_trans_obj {
>>>  	struct nft_object		*obj;
>>> +	struct nlattr			**tb;
>>
>> Instead of annotatint tb[] on the object, you can probably add here:
>>
>> union {
>>         struct quota {
>>                 uint64_t                consumed;
>>                 uint64_t                quota;
>>       } quota;
>> };
>>
>> So the initial update annotates the values in the transaction.
>>
>> I guess you will need two new indirections? Something like
>> prepare_update() and update().
> 
> Or you have a single update() and pass enum nft_trans_phase as
> parameter, so this only needs one single indirection.
> 

But also we would need to continue passing the 'bool commit' as a
parameter too right? I will take a look to nft_trans_phase. Thanks! :-)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-23 18:05     ` Fernando Fernandez Mancera
@ 2019-08-23 18:28       ` Fernando Fernandez Mancera
  2019-08-24 16:29         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-23 18:28 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
> 
> 
> On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
>> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
>>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
>>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>>>>  
>>>>  struct nft_trans_obj {
>>>>  	struct nft_object		*obj;
>>>> +	struct nlattr			**tb;
>>>
>>> Instead of annotatint tb[] on the object, you can probably add here:
>>>
>>> union {
>>>         struct quota {
>>>                 uint64_t                consumed;
>>>                 uint64_t                quota;
>>>       } quota;
>>> };
>>>
>>> So the initial update annotates the values in the transaction.
>>>

If we follow that pattern then the indirection would need the
nft_trans_phase enum, the quota struct and also the tb[] as parameters
because in the preparation phase we always need the tb[] array.

Why is that better than annotating tb[] on the object? Sorry, I think
that I am missing something here. Thanks!

>>> I guess you will need two new indirections? Something like
>>> prepare_update() and update().
>>
>> Or you have a single update() and pass enum nft_trans_phase as
>> parameter, so this only needs one single indirection.
>>
> 
> But also we would need to continue passing the 'bool commit' as a
> parameter too right? I will take a look to nft_trans_phase. Thanks! :-)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-23 18:28       ` Fernando Fernandez Mancera
@ 2019-08-24 16:29         ` Pablo Neira Ayuso
  2019-08-26 10:16           ` Fernando Fernandez Mancera
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-24 16:29 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Fri, Aug 23, 2019 at 08:28:46PM +0200, Fernando Fernandez Mancera wrote:
> On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
> > 
> > 
> > On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
> >> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
> >>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
> >>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
> >>>>  
> >>>>  struct nft_trans_obj {
> >>>>  	struct nft_object		*obj;
> >>>> +	struct nlattr			**tb;
> >>>
> >>> Instead of annotatint tb[] on the object, you can probably add here:
> >>>
> >>> union {
> >>>         struct quota {
> >>>                 uint64_t                consumed;
> >>>                 uint64_t                quota;
> >>>       } quota;
> >>> };
> >>>
> >>> So the initial update annotates the values in the transaction.
> >>>
> 
> If we follow that pattern then the indirection would need the
> nft_trans_phase enum, the quota struct and also the tb[] as parameters
> because in the preparation phase we always need the tb[] array.

Right, so this is my next idea :-)

For the update case, I'd suggest you use the existing 'obj' field in
the transaction object. The idea would be to allocate a new object via
nft_obj_init() from the update path. Hence, you can use the existing
expr->ops->init() interface to parse the attributes - I find the
existing parsing for ->update() a bit redundant.

Then, from the commit path, you use the new ->update() interface to
update the object accordingly taking this new object as input. I think
you cannot update u64 quota like you do in this patch. On 32-bit
arches, an assignment of u64 won't be atomic. So you have to use
atomic64_set() and atomic64_read() to make sure that packet path does
not observes an inconsistent state. BTW, once you have updated the
existing object, you can just release the object in the transaction
coming in this update. I think you will need a 'bool update' field on
the transaction object, so the commit path knows how to handle the
update.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-24 16:29         ` Pablo Neira Ayuso
@ 2019-08-26 10:16           ` Fernando Fernandez Mancera
  2019-08-26 10:31             ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-26 10:16 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Hi Pablo,

On 8/24/19 6:29 PM, Pablo Neira Ayuso wrote:
> On Fri, Aug 23, 2019 at 08:28:46PM +0200, Fernando Fernandez Mancera wrote:
>> On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
>>>
>>>
>>> On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
>>>> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
>>>>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
>>>>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>>>>>>  
>>>>>>  struct nft_trans_obj {
>>>>>>  	struct nft_object		*obj;
>>>>>> +	struct nlattr			**tb;
>>>>>
>>>>> Instead of annotatint tb[] on the object, you can probably add here:
>>>>>
>>>>> union {
>>>>>         struct quota {
>>>>>                 uint64_t                consumed;
>>>>>                 uint64_t                quota;
>>>>>       } quota;
>>>>> };
>>>>>
>>>>> So the initial update annotates the values in the transaction.
>>>>>
>>
>> If we follow that pattern then the indirection would need the
>> nft_trans_phase enum, the quota struct and also the tb[] as parameters
>> because in the preparation phase we always need the tb[] array.
> 
> Right, so this is my next idea :-)
> 
> For the update case, I'd suggest you use the existing 'obj' field in
> the transaction object. The idea would be to allocate a new object via
> nft_obj_init() from the update path. Hence, you can use the existing
> expr->ops->init() interface to parse the attributes - I find the
> existing parsing for ->update() a bit redundant.
> 
> Then, from the commit path, you use the new ->update() interface to
> update the object accordingly taking this new object as input. I think
> you cannot update u64 quota like you do in this patch. On 32-bit
> arches, an assignment of u64 won't be atomic. So you have to use
> atomic64_set() and atomic64_read() to make sure that packet path does
> not observes an inconsistent state. BTW, once you have updated the
> existing object, you can just release the object in the transaction
> coming in this update. I think you will need a 'bool update' field on
> the transaction object, so the commit path knows how to handle the
> update.
> 

I would need to add a second 'obj' field in the transaction object in
order to have the existing object pointer and the new one.

Also, right now we are not using atomic64_set() when initializing u64
quota is that a bug then? If it is, I could include a patch fixing it.

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-26 10:16           ` Fernando Fernandez Mancera
@ 2019-08-26 10:31             ` Pablo Neira Ayuso
  2019-08-26 10:38               ` Fernando Fernandez Mancera
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-26 10:31 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Mon, Aug 26, 2019 at 12:16:34PM +0200, Fernando Fernandez Mancera wrote:
> Hi Pablo,
> 
> On 8/24/19 6:29 PM, Pablo Neira Ayuso wrote:
> > On Fri, Aug 23, 2019 at 08:28:46PM +0200, Fernando Fernandez Mancera wrote:
> >> On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
> >>>
> >>>
> >>> On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
> >>>> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
> >>>>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
> >>>>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
> >>>>>>  
> >>>>>>  struct nft_trans_obj {
> >>>>>>  	struct nft_object		*obj;
> >>>>>> +	struct nlattr			**tb;
> >>>>>
> >>>>> Instead of annotatint tb[] on the object, you can probably add here:
> >>>>>
> >>>>> union {
> >>>>>         struct quota {
> >>>>>                 uint64_t                consumed;
> >>>>>                 uint64_t                quota;
> >>>>>       } quota;
> >>>>> };
> >>>>>
> >>>>> So the initial update annotates the values in the transaction.
> >>>>>
> >>
> >> If we follow that pattern then the indirection would need the
> >> nft_trans_phase enum, the quota struct and also the tb[] as parameters
> >> because in the preparation phase we always need the tb[] array.
> > 
> > Right, so this is my next idea :-)
> > 
> > For the update case, I'd suggest you use the existing 'obj' field in
> > the transaction object. The idea would be to allocate a new object via
> > nft_obj_init() from the update path. Hence, you can use the existing
> > expr->ops->init() interface to parse the attributes - I find the
> > existing parsing for ->update() a bit redundant.
> > 
> > Then, from the commit path, you use the new ->update() interface to
> > update the object accordingly taking this new object as input. I think
> > you cannot update u64 quota like you do in this patch. On 32-bit
> > arches, an assignment of u64 won't be atomic. So you have to use
> > atomic64_set() and atomic64_read() to make sure that packet path does
> > not observes an inconsistent state. BTW, once you have updated the
> > existing object, you can just release the object in the transaction
> > coming in this update. I think you will need a 'bool update' field on
> > the transaction object, so the commit path knows how to handle the
> > update.
> > 
> 
> I would need to add a second 'obj' field in the transaction object in
> order to have the existing object pointer and the new one.

Hm, this might be convenient, we might need to swap other objects.

> Also, right now we are not using atomic64_set() when initializing u64
> quota is that a bug then? If it is, I could include a patch fixing it.

No packets see that quota limit by when it is set for first time.

With quota updates, packets are walking over this state while this is
updated from the control plane.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation
  2019-08-26 10:31             ` Pablo Neira Ayuso
@ 2019-08-26 10:38               ` Fernando Fernandez Mancera
  0 siblings, 0 replies; 10+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-26 10:38 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel



On 8/26/19 12:31 PM, Pablo Neira Ayuso wrote:
> On Mon, Aug 26, 2019 at 12:16:34PM +0200, Fernando Fernandez Mancera wrote:
>> Hi Pablo,
>>
>> On 8/24/19 6:29 PM, Pablo Neira Ayuso wrote:
>>> On Fri, Aug 23, 2019 at 08:28:46PM +0200, Fernando Fernandez Mancera wrote:
>>>> On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
>>>>>
>>>>>
>>>>> On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
>>>>>> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
>>>>>>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera wrote:
>>>>>>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>>>>>>>>  
>>>>>>>>  struct nft_trans_obj {
>>>>>>>>  	struct nft_object		*obj;
>>>>>>>> +	struct nlattr			**tb;
>>>>>>>
>>>>>>> Instead of annotatint tb[] on the object, you can probably add here:
>>>>>>>
>>>>>>> union {
>>>>>>>         struct quota {
>>>>>>>                 uint64_t                consumed;
>>>>>>>                 uint64_t                quota;
>>>>>>>       } quota;
>>>>>>> };
>>>>>>>
>>>>>>> So the initial update annotates the values in the transaction.
>>>>>>>
>>>>
>>>> If we follow that pattern then the indirection would need the
>>>> nft_trans_phase enum, the quota struct and also the tb[] as parameters
>>>> because in the preparation phase we always need the tb[] array.
>>>
>>> Right, so this is my next idea :-)
>>>
>>> For the update case, I'd suggest you use the existing 'obj' field in
>>> the transaction object. The idea would be to allocate a new object via
>>> nft_obj_init() from the update path. Hence, you can use the existing
>>> expr->ops->init() interface to parse the attributes - I find the
>>> existing parsing for ->update() a bit redundant.
>>>
>>> Then, from the commit path, you use the new ->update() interface to
>>> update the object accordingly taking this new object as input. I think
>>> you cannot update u64 quota like you do in this patch. On 32-bit
>>> arches, an assignment of u64 won't be atomic. So you have to use
>>> atomic64_set() and atomic64_read() to make sure that packet path does
>>> not observes an inconsistent state. BTW, once you have updated the
>>> existing object, you can just release the object in the transaction
>>> coming in this update. I think you will need a 'bool update' field on
>>> the transaction object, so the commit path knows how to handle the
>>> update.
>>>
>>
>> I would need to add a second 'obj' field in the transaction object in
>> order to have the existing object pointer and the new one.
> 
> Hm, this might be convenient, we might need to swap other objects.
> 
>> Also, right now we are not using atomic64_set() when initializing u64
>> quota is that a bug then? If it is, I could include a patch fixing it.
> 
> No packets see that quota limit by when it is set for first time.
> 
> With quota updates, packets are walking over this state while this is
> updated from the control plane.
> 

Thanks for explaining. I am preparing a patch. Thanks Pablo!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-08-26 10:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-22 16:48 [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Fernando Fernandez Mancera
2019-08-22 16:48 ` [PATCH 2/2 nf-next v2] netfilter: nft_quota: add quota object update support Fernando Fernandez Mancera
2019-08-23 12:41 ` [PATCH 1/2 nf-next v2] netfilter: nf_tables: Introduce stateful object update operation Pablo Neira Ayuso
2019-08-23 12:42   ` Pablo Neira Ayuso
2019-08-23 18:05     ` Fernando Fernandez Mancera
2019-08-23 18:28       ` Fernando Fernandez Mancera
2019-08-24 16:29         ` Pablo Neira Ayuso
2019-08-26 10:16           ` Fernando Fernandez Mancera
2019-08-26 10:31             ` Pablo Neira Ayuso
2019-08-26 10:38               ` Fernando Fernandez Mancera

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).