netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC nf-next] Introducing stateful object update operation
@ 2019-08-06 10:29 Fernando Fernandez Mancera
  2019-08-06 10:44 ` Florian Westphal
  2019-08-06 11:06 ` Pablo Neira Ayuso
  0 siblings, 2 replies; 5+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-06 10:29 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Fernando Fernandez Mancera

I have been thinking of a way to update a quota object. i.e raise or lower the
quota limit of an existing object. I think it would be ideal to implement the
operations of updating objects in the API in a generic way.

Therefore, we could easily give update support to each object type by adding an
update operation in the "nft_object_ops" struct. This is a conceptual patch so
it does not work.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 include/net/netfilter/nf_tables.h        |  4 ++++
 include/uapi/linux/netfilter/nf_tables.h |  2 ++
 net/netfilter/nf_tables_api.c            | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 9b624566b82d..bd1e6c19d23f 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1101,6 +1101,7 @@ struct nft_object_type {
  *	@eval: stateful object evaluation function
  *	@size: stateful object size
  *	@init: initialize object from netlink attributes
+ *	@update: update object from netlink attributes
  *	@destroy: release existing stateful object
  *	@dump: netlink dump stateful object
  */
@@ -1112,6 +1113,9 @@ struct nft_object_ops {
 	int				(*init)(const struct nft_ctx *ctx,
 						const struct nlattr *const tb[],
 						struct nft_object *obj);
+	int				(*update)(const struct nft_ctx *ctx,
+						  const struct nlattr *const tb[],
+						  struct nft_object *obj);
 	void				(*destroy)(const struct nft_ctx *ctx,
 						   struct nft_object *obj);
 	int				(*dump)(struct sk_buff *skb,
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 82abaa183fc3..8b0a012e9177 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -92,6 +92,7 @@ enum nft_verdicts {
  * @NFT_MSG_NEWOBJ: create a stateful object (enum nft_obj_attributes)
  * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes)
  * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes)
+ * @NFT_MSG_UPDOBJ: update a stateful object (enum nft_obj_attributes)
  * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes)
  * @NFT_MSG_NEWFLOWTABLE: add new flow table (enum nft_flowtable_attributes)
  * @NFT_MSG_GETFLOWTABLE: get flow table (enum nft_flowtable_attributes)
@@ -119,6 +120,7 @@ enum nf_tables_msg_types {
 	NFT_MSG_NEWOBJ,
 	NFT_MSG_GETOBJ,
 	NFT_MSG_DELOBJ,
+	NFT_MSG_UPDOBJ,
 	NFT_MSG_GETOBJ_RESET,
 	NFT_MSG_NEWFLOWTABLE,
 	NFT_MSG_GETFLOWTABLE,
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 605a7cfe7ca7..c7267f418808 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5420,6 +5420,16 @@ static void nft_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
 	kfree(obj);
 }
 
+static int nf_tables_updobj(struct net *net, struct sock *nlsk,
+			    struct sk_buff *skb, const struct nlmsghdr *nlh,
+			    const struct nlattr * const nla[],
+			    struct netlink_ext_ack *extack)
+{
+	/* Placeholder function, here we would need to check if the object
+	 * exists. Then init the context and update the object.*/
+	return 1;
+}
+
 static int nf_tables_delobj(struct net *net, struct sock *nlsk,
 			    struct sk_buff *skb, const struct nlmsghdr *nlh,
 			    const struct nlattr * const nla[],
@@ -6323,6 +6333,11 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
 		.attr_count	= NFTA_OBJ_MAX,
 		.policy		= nft_obj_policy,
 	},
+	[NFT_MSG_UPDOBJ] = {
+		.call_rcu	= nf_tables_updobj,
+		.attr_count	= NFTA_OBJ_MAX,
+		.policy		= nft_obj_policy,
+	},
 	[NFT_MSG_DELOBJ] = {
 		.call_batch	= nf_tables_delobj,
 		.attr_count	= NFTA_OBJ_MAX,
@@ -6791,6 +6806,10 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 					     NFT_MSG_NEWOBJ);
 			nft_trans_destroy(trans);
 			break;
+		case NFT_MSG_UPDOBJ:
+			nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
+					     NFT_MSG_UPDOBJ);
+			break;
 		case NFT_MSG_DELOBJ:
 			nft_obj_del(nft_trans_obj(trans));
 			nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
@@ -6939,6 +6958,9 @@ static int __nf_tables_abort(struct net *net)
 			trans->ctx.table->use--;
 			nft_obj_del(nft_trans_obj(trans));
 			break;
+		case NFT_MSG_UPDOBJ:
+			nft_trans_destroy(trans);
+			break;
 		case NFT_MSG_DELOBJ:
 			trans->ctx.table->use++;
 			nft_clear(trans->ctx.net, nft_trans_obj(trans));
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH RFC nf-next] Introducing stateful object update operation
  2019-08-06 10:29 [PATCH RFC nf-next] Introducing stateful object update operation Fernando Fernandez Mancera
@ 2019-08-06 10:44 ` Florian Westphal
  2019-08-06 11:06 ` Pablo Neira Ayuso
  1 sibling, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2019-08-06 10:44 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> I have been thinking of a way to update a quota object. i.e raise or lower the
> quota limit of an existing object. I think it would be ideal to implement the
> operations of updating objects in the API in a generic way.
> 
> Therefore, we could easily give update support to each object type by adding an
> update operation in the "nft_object_ops" struct. This is a conceptual patch so
> it does not work.
> 
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  include/net/netfilter/nf_tables.h        |  4 ++++
>  include/uapi/linux/netfilter/nf_tables.h |  2 ++
>  net/netfilter/nf_tables_api.c            | 22 ++++++++++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 9b624566b82d..bd1e6c19d23f 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1101,6 +1101,7 @@ struct nft_object_type {
>   *	@eval: stateful object evaluation function
>   *	@size: stateful object size
>   *	@init: initialize object from netlink attributes
> + *	@update: update object from netlink attributes
>   *	@destroy: release existing stateful object
>   *	@dump: netlink dump stateful object
>   */
> @@ -1112,6 +1113,9 @@ struct nft_object_ops {
>  	int				(*init)(const struct nft_ctx *ctx,
>  						const struct nlattr *const tb[],
>  						struct nft_object *obj);
> +	int				(*update)(const struct nft_ctx *ctx,
> +						  const struct nlattr *const tb[],
> +						  struct nft_object *obj);
>  	void				(*destroy)(const struct nft_ctx *ctx,
>  						   struct nft_object *obj);
>  	int				(*dump)(struct sk_buff *skb,
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 82abaa183fc3..8b0a012e9177 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -92,6 +92,7 @@ enum nft_verdicts {
>   * @NFT_MSG_NEWOBJ: create a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes)
> + * @NFT_MSG_UPDOBJ: update a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_NEWFLOWTABLE: add new flow table (enum nft_flowtable_attributes)
>   * @NFT_MSG_GETFLOWTABLE: get flow table (enum nft_flowtable_attributes)
> @@ -119,6 +120,7 @@ enum nf_tables_msg_types {
>  	NFT_MSG_NEWOBJ,
>  	NFT_MSG_GETOBJ,
>  	NFT_MSG_DELOBJ,
> +	NFT_MSG_UPDOBJ,

This breaks ABI, new enums need to be added at the end.

But I wonder if we can't just re-use NEWOBJ and teach it to update
the object if it exists already.

Userspace can already pass EXCL flag to bail out for the 'exists' case.

I agree that such feature (object update) is a good idea.

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH RFC nf-next] Introducing stateful object update operation
  2019-08-06 10:29 [PATCH RFC nf-next] Introducing stateful object update operation Fernando Fernandez Mancera
  2019-08-06 10:44 ` Florian Westphal
@ 2019-08-06 11:06 ` Pablo Neira Ayuso
  2019-08-06 13:40   ` Fernando Fernandez Mancera
  1 sibling, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-06 11:06 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Tue, Aug 06, 2019 at 12:29:45PM +0200, Fernando Fernandez Mancera wrote:
> I have been thinking of a way to update a quota object. i.e raise or lower the
> quota limit of an existing object. I think it would be ideal to implement the
> operations of updating objects in the API in a generic way.
> 
> Therefore, we could easily give update support to each object type by adding an
> update operation in the "nft_object_ops" struct. This is a conceptual patch so
> it does not work.
> 
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  include/net/netfilter/nf_tables.h        |  4 ++++
>  include/uapi/linux/netfilter/nf_tables.h |  2 ++
>  net/netfilter/nf_tables_api.c            | 22 ++++++++++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 9b624566b82d..bd1e6c19d23f 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1101,6 +1101,7 @@ struct nft_object_type {
>   *	@eval: stateful object evaluation function
>   *	@size: stateful object size
>   *	@init: initialize object from netlink attributes
> + *	@update: update object from netlink attributes
>   *	@destroy: release existing stateful object
>   *	@dump: netlink dump stateful object
>   */
> @@ -1112,6 +1113,9 @@ struct nft_object_ops {
>  	int				(*init)(const struct nft_ctx *ctx,
>  						const struct nlattr *const tb[],
>  						struct nft_object *obj);
> +	int				(*update)(const struct nft_ctx *ctx,
> +						  const struct nlattr *const tb[],
> +						  struct nft_object *obj);
>  	void				(*destroy)(const struct nft_ctx *ctx,
>  						   struct nft_object *obj);
>  	int				(*dump)(struct sk_buff *skb,
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 82abaa183fc3..8b0a012e9177 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -92,6 +92,7 @@ enum nft_verdicts {
>   * @NFT_MSG_NEWOBJ: create a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes)
> + * @NFT_MSG_UPDOBJ: update a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes)
>   * @NFT_MSG_NEWFLOWTABLE: add new flow table (enum nft_flowtable_attributes)
>   * @NFT_MSG_GETFLOWTABLE: get flow table (enum nft_flowtable_attributes)
> @@ -119,6 +120,7 @@ enum nf_tables_msg_types {
>  	NFT_MSG_NEWOBJ,
>  	NFT_MSG_GETOBJ,
>  	NFT_MSG_DELOBJ,
> +	NFT_MSG_UPDOBJ,

No need for this new message type, see below.

>  	NFT_MSG_GETOBJ_RESET,
>  	NFT_MSG_NEWFLOWTABLE,
>  	NFT_MSG_GETFLOWTABLE,
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 605a7cfe7ca7..c7267f418808 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -5420,6 +5420,16 @@ static void nft_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
>  	kfree(obj);
>  }
>  
> +static int nf_tables_updobj(struct net *net, struct sock *nlsk,
> +			    struct sk_buff *skb, const struct nlmsghdr *nlh,
> +			    const struct nlattr * const nla[],
> +			    struct netlink_ext_ack *extack)
> +{
> +	/* Placeholder function, here we would need to check if the object
> +	 * exists. Then init the context and update the object.*/
> +	return 1;

Use the existing nf_tables_newobj(), if NLM_F_EXCL is not set on and
the object exists, then this is an update.

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH RFC nf-next] Introducing stateful object update operation
  2019-08-06 11:06 ` Pablo Neira Ayuso
@ 2019-08-06 13:40   ` Fernando Fernandez Mancera
  2019-08-06 13:48     ` Florian Westphal
  0 siblings, 1 reply; 5+ messages in thread
From: Fernando Fernandez Mancera @ 2019-08-06 13:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel



On 8/6/19 1:06 PM, Pablo Neira Ayuso wrote:
> On Tue, Aug 06, 2019 at 12:29:45PM +0200, Fernando Fernandez Mancera wrote:
>> I have been thinking of a way to update a quota object. i.e raise or lower the
>> quota limit of an existing object. I think it would be ideal to implement the
>> operations of updating objects in the API in a generic way.
>>
>> Therefore, we could easily give update support to each object type by adding an
>> update operation in the "nft_object_ops" struct. This is a conceptual patch so
>> it does not work.
>>
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>>  include/net/netfilter/nf_tables.h        |  4 ++++
>>  include/uapi/linux/netfilter/nf_tables.h |  2 ++
>>  net/netfilter/nf_tables_api.c            | 22 ++++++++++++++++++++++
>>  3 files changed, 28 insertions(+)
>>
>> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
>> index 9b624566b82d..bd1e6c19d23f 100644
>> --- a/include/net/netfilter/nf_tables.h
>> +++ b/include/net/netfilter/nf_tables.h
>> @@ -1101,6 +1101,7 @@ struct nft_object_type {
>>   *	@eval: stateful object evaluation function
>>   *	@size: stateful object size
>>   *	@init: initialize object from netlink attributes
>> + *	@update: update object from netlink attributes
>>   *	@destroy: release existing stateful object
>>   *	@dump: netlink dump stateful object
>>   */
>> @@ -1112,6 +1113,9 @@ struct nft_object_ops {
>>  	int				(*init)(const struct nft_ctx *ctx,
>>  						const struct nlattr *const tb[],
>>  						struct nft_object *obj);
>> +	int				(*update)(const struct nft_ctx *ctx,
>> +						  const struct nlattr *const tb[],
>> +						  struct nft_object *obj);
>>  	void				(*destroy)(const struct nft_ctx *ctx,
>>  						   struct nft_object *obj);
>>  	int				(*dump)(struct sk_buff *skb,
>> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
>> index 82abaa183fc3..8b0a012e9177 100644
>> --- a/include/uapi/linux/netfilter/nf_tables.h
>> +++ b/include/uapi/linux/netfilter/nf_tables.h
>> @@ -92,6 +92,7 @@ enum nft_verdicts {
>>   * @NFT_MSG_NEWOBJ: create a stateful object (enum nft_obj_attributes)
>>   * @NFT_MSG_GETOBJ: get a stateful object (enum nft_obj_attributes)
>>   * @NFT_MSG_DELOBJ: delete a stateful object (enum nft_obj_attributes)
>> + * @NFT_MSG_UPDOBJ: update a stateful object (enum nft_obj_attributes)
>>   * @NFT_MSG_GETOBJ_RESET: get and reset a stateful object (enum nft_obj_attributes)
>>   * @NFT_MSG_NEWFLOWTABLE: add new flow table (enum nft_flowtable_attributes)
>>   * @NFT_MSG_GETFLOWTABLE: get flow table (enum nft_flowtable_attributes)
>> @@ -119,6 +120,7 @@ enum nf_tables_msg_types {
>>  	NFT_MSG_NEWOBJ,
>>  	NFT_MSG_GETOBJ,
>>  	NFT_MSG_DELOBJ,
>> +	NFT_MSG_UPDOBJ,
> 
> No need for this new message type, see below.
> 
>>  	NFT_MSG_GETOBJ_RESET,
>>  	NFT_MSG_NEWFLOWTABLE,
>>  	NFT_MSG_GETFLOWTABLE,
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index 605a7cfe7ca7..c7267f418808 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -5420,6 +5420,16 @@ static void nft_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
>>  	kfree(obj);
>>  }
>>  
>> +static int nf_tables_updobj(struct net *net, struct sock *nlsk,
>> +			    struct sk_buff *skb, const struct nlmsghdr *nlh,
>> +			    const struct nlattr * const nla[],
>> +			    struct netlink_ext_ack *extack)
>> +{
>> +	/* Placeholder function, here we would need to check if the object
>> +	 * exists. Then init the context and update the object.*/
>> +	return 1;
> 
> Use the existing nf_tables_newobj(), if NLM_F_EXCL is not set on and
> the object exists, then this is an update.
> 

I agree on that. But I think that if we use the NFT_MSG_NEWOBJ there
will be some issues in the commit and the abort phase. That is why I
think "NFT_MSG_UPDOBJ" would be needed.

Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH RFC nf-next] Introducing stateful object update operation
  2019-08-06 13:40   ` Fernando Fernandez Mancera
@ 2019-08-06 13:48     ` Florian Westphal
  0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2019-08-06 13:48 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: Pablo Neira Ayuso, netfilter-devel

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> > Use the existing nf_tables_newobj(), if NLM_F_EXCL is not set on and
> > the object exists, then this is an update.
> 
> I agree on that. But I think that if we use the NFT_MSG_NEWOBJ there
> will be some issues in the commit and the abort phase. That is why I
> think "NFT_MSG_UPDOBJ" would be needed.

See e.g. 'nft_trans_table_update()' -- we already do this for
other structures/entities.  You would need to extend the object handling
to not remove an already-existed-object in case of an update if an abort
is triggered.

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-06 13:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 10:29 [PATCH RFC nf-next] Introducing stateful object update operation Fernando Fernandez Mancera
2019-08-06 10:44 ` Florian Westphal
2019-08-06 11:06 ` Pablo Neira Ayuso
2019-08-06 13:40   ` Fernando Fernandez Mancera
2019-08-06 13:48     ` Florian Westphal

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).