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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.