Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> This patch adds a timeout marker for those elements that never expire
> when the element are created, so timeout updates are possible.
> 
> Note that maximum supported timeout in milliseconds which is conveyed
> within a netlink attribute is 0x10c6f7a0b5ec which translates to
> 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> element timeout field for permanent elements.
> 
> If userspace provides no timeout for an element, then the default set
> timeout applies. However, if no default set timeout is specified and
> timeout flag is set on, then such new element gets the never expires
> marker.
> 
> Note that, in older kernels, it is already possible to define elements
> that never expire by declaring a set with the set timeout flag set on
> and no global set timeout, in this case, new element with no explicit
> timeout never expire do not allocate the timeout extension, hence, they
> never expire. This approach makes it complicated to accomodate element
> timeout update, because element extensions do not support reallocations.
> Therefore, allocate the timeout extension and use the new marker for
> this case, but do not expose it to userspace to retain backward
> compatibility in the set listing.

I fail to miss the point why this marker is needed at all:

Right now, new set elements receive EXT_TIMEOUT upon creation if either
NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
timeout) or set->timeout is non-zero (i.e., set has a default timeout).

Why not change this logic and add EXT_TIMEOUT to all elements which will
timeout and initialize it either to the user-defined value or the set's
default? Then, only elements which don't timeout remain without
EXT_TIMEOUT. Which is not a problem, because their expiration value does
not need to be reset and thus they don't need space for one.

The only downside is that elements which stick to the set's default
timeout increase in size (for the copied set->timeout value), but this
patch series merges EXT_EXPIRATION and EXT_TIMEOUT anyway so their size
increases already. OTOH, non-expiring set elements won't need a spurious
EXT_TIMEOUT holding never expires marker.

Cheers, Phil