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

[Fernando Fernandez Mancera <ffmancera@riseup.net>]

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!