Re: [PATCH nf-next v2 1/8] netfilter: nf_tables: Support for subkeys, set with multiple ranged fields

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

On Mon, Nov 25, 2019 at 03:54:22PM +0100, Stefano Brivio wrote:
> On Mon, 25 Nov 2019 15:30:58 +0100
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Mon, Nov 25, 2019 at 02:26:16PM +0100, Stefano Brivio wrote:
> > > On Mon, 25 Nov 2019 10:58:17 +0100
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >   
> > > > On Mon, Nov 25, 2019 at 10:30:35AM +0100, Stefano Brivio wrote:
> > > > [...]  
> > > > > Another idea could be that we get rid of this flag altogether: if we
> > > > > move "subkeys" to set->desc, the ->estimate() functions of rbtree and
> > > > > pipapo can check for those and refuse or allow set selection
> > > > > accordingly. I have no idea yet if this introduces further complexity
> > > > > for nft, because there we would need to decide how to create start/end
> > > > > elements depending on the existing set description instead of using a
> > > > > single flag. I can give it a try if it makes sense.    
> > > > 
> > > > nft_set_desc can probably store a boolean 'concat' that is set on if
> > > > the NFTA_SET_DESC_SUBKEY attribute is specified. Then, this flag is
> > > > not needed and you can just rely on ->estimate() as you describe.  
> > > 
> > > I could even just check desc->num_subkeys from your patch then, without
> > > adding another field to nft_set_desc. Too ugly?  
> > 
> > OK.
> > 
> > > > The hashtable will just ignore this description, it does not need the
> > > > description even if userspace pass it on since the interval flag is
> > > > set on.
> > > > 
> > > > You just have to update the rbtree to check for desc->concat, if this
> > > > is true, then rbtree->estimate() returns false.  
> > > 
> > > Yes, I think it all makes sense, thanks for detailing the idea. I'll get
> > > to this in a few hours.
> > >   
> > > > BTW, then probably you can rename this attribute to
> > > > NFT_SET_DESC_CONCAT?  
> > > 
> > > It would include sizes, though. What about NFT_SET_DESC_SUBSIZE or
> > > NFT_SET_DESC_FIELD_SIZE?  
> > 
> > You mean this:
> > 
> >        NFT_SET_DESC_SUBSIZE
> >           NFT_SET_DESC_FIELD_SIZE
> >           NFT_SET_DESC_FIELD_SIZE
> > 
> > instead of this:
> > 
> >         NFT_SET_DESC_CONCAT
> >           NFT_LIST_ELEM
> >              NFT_SET_DESC_SUBKEY_LEN
> >           NFT_LIST_ELEM
> >              NFT_SET_DESC_SUBKEY_LEN
> > 
> > If I described this correctly, your approach is more simple indeed.
> 
> Ah, yes, that's what I meant, but that's because I didn't understand
> your intention in the first place. :) I see now.
> 
> > However, I don't really have specific requirements for the future
> > right now. The one below is leaving room to add more subkey fields (to
> > describe each subkey if that is ever required). My experience is that
> > leaving room to extend netlink in the future is usually a good idea,
> > that's all.
> > 
> > Instead of NFT_LIST_ELEM, something like NFT_SET_DESC_SUBKEY should be
> > fine too, ie.
> > 
> >         NFT_SET_DESC_CONCAT
> >           NFT_SET_DESC_SUBKEY
> >              NFT_SET_DESC_SUBKEY_LEN
> >           NFT_SET_DESC_SUBKEY
> >              NFT_SET_DESC_SUBKEY_LEN
> 
> Actually:
> 
> >         NFT_SET_DESC_CONCAT
> >           NFT_LIST_ELEM
> >              NFT_SET_DESC_SUBKEY_LEN
> >           NFT_LIST_ELEM
> >              NFT_SET_DESC_SUBKEY_LEN
> 
> sounds better to me. Maybe "SUBKEY" starts looking a bit obscure here:
> the "SUB" part is already there, the "KEY" part mostly refers to an
> implementation detail. What about:
> 
>          NFT_SET_DESC_CONCAT
>            NFT_LIST_ELEM
>               NFT_SET_DESC_LEN
>            NFT_LIST_ELEM
>               NFT_SET_DESC_LEN
> 
> this?

I think the _SUBKEY_ infix is fine.

Problem with using NFT_SET_DESC_* for inner (nested) attributes is
that this matches the same prefix as top level netlink attributes.

Note there is NFT_SET_DESC_SIZE. If there is a NFT_SET_DESC_LEN that
is wrapped around NFT_SET_DESC_CONCAT (using same prefix) it might
look a bit misleading.

Having said this, although I started this debate about naming, I don't
have a strong opinion on all these names. As soon as the netlink
attribute scheme that is used allows for extensibility in the future,
such as allowing to add more descriptions for each subkey, I'll be fine.

Thanks.