Re: [PATCH nf] netfilter: nf_tables: do not refresh timeout when resetting element

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

On Mon, Oct 02, 2023 at 11:50:25PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 02, 2023 at 08:06:42PM +0200, Phil Sutter wrote:
> > On Mon, Oct 02, 2023 at 11:05:16AM +0200, Pablo Neira Ayuso wrote:
> > > The dump and reset command should not refresh the timeout, this command
> > > is intended to allow users to list existing stateful objects and reset
> > > them, element expiration should be refresh via transaction instead with
> > > a specific command to achieve this, otherwise this is entering combo
> > > semantics that will be hard to be undone later (eg. a user asking to
> > > retrieve counters but _not_ requiring to refresh expiration).
> > 
> > From a users' perspective, what is special about the element expires
> > value disqualifying it from being reset along with any counter/quota
> > values?
> >
> > Do you have a PoC for set element reset via transaction yet? Can we
> > integrate non-timeout resets with it, too? Because IIUC, that's an
> > alternative to the pending reset locking.
> 
> Problem is listing is not supported from transaction path, this is
> using existing netlink dump infrastructure which runs lockless via
> rcu. So we could support reset, but we could not use netlink dump
> semantics to fetch the values, and user likely wants this to
> fetch-and-reset as in ctnetlink.

Well, with NLM_F_ECHO, it should be possible to explore reset under
commit_mutex, but is it really worth the effort?

With two concurrent threads, we just want to ensure that no invalid
state shows in the listing (you mentioned it is possible to list
negative values with two threads listing-and-resetting at the same
time).

I think we should just make sure something valid is included in the
listing, but as for the two threads performing list-and-reset, why
ensure strict serialization?

This is a rare operation to fetch statistics, most likely having a
single process performing this in place? So we are currently
discussing how to fix the (negative) non-sense in the listing.

> > What we have now is a broad 'reset element', not specifying what to
> > reset. If the above is a feature being asked for, I'd rather implement
> > 'reset element counter', 'reset element timeout', 'reset element quota',
> > etc. commands.
> 
> We are currently discussing how to implement refresh timeout into the
> transaction model.
> 
> I would suggest we keep this chunk away by now for the _RESET command,
> until we agree on next steps.