| Message ID | 20190806102945.728-1-ffmancera@riseup.net |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [RFC,nf-next] Introducing stateful object update operation | expand |
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.
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.
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!
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.
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));
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(+)