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

[Phil Sutter <phil@nwl.cc>]

On Tue, Aug 13, 2024 at 10:43:44PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 13, 2024 at 08:12:18PM +0200, Phil Sutter wrote:
> > 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:
> 
> Long story short: I did my best to support this without this marker
> but I could not find a design that works without it.
> 
> > 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).
> 
> There is one exception though:
> 
>  table inet x {
>         set y {
>                 typeof ip saddr
>                 flags timeout
>         }
>  }
> 
> in this case, there is no default set timeout. Older kernels already
> allow to add elements with no EXT_TIMEOUT that never expire with this
> approach, however, this is not practical for element updates, because
> set element extension reallocation is not supported.
> 
> > 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.
> 
> No EXT_TIMEOUT means users cannot update the timeout policy for such
> element. I assume users can update from "timeout never" to "timeout 1h"
> as a valid usecase.

Ah, that's the missing piece: I somehow assumed this should only support
resetting elements' timeouts, i.e. update only those elements which will
timeout already.

AFAICT, using UINT64_MAX as never-timeout marker is sane but can't one
use 0 instead? Set elems expire if EXT_TIMEOUT is present and 'timeout
!= 0'. This should simplify the code a bit, too because one may default
to set->timeout without checking the actual value.

Thanks, Phil