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

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

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.