Re: [PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[Florian Westphal]

Joas Antonio dos Santos <joasantonio108@xxxxxxxxx> wrote:
> The catch is that the expiration isn't stamped at activation, it's
> stamped when the element is allocated. nft_set_elem_init() does:
> 
> 	nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;
> 
> and that happens back in the parse phase (nft_add_set_elem), while we're
> still going through the batch. The element only goes active later, in
> nf_tables_commit(), which is also where the gc tstamp is taken
> (nft_net->tstamp = get_jiffies_64()).


Can you name the line in the source code?
I am not seeing it :-(

> So the expiration is really parse_time + timeout, and the commit-time gc
> scan checks it against commit_time. If you add a big batch of elements
> with a short timeout, they get parsed one by one and the commit only
> happens at the very end, so the ones parsed early in the batch have
> already gone past their timeout by the time we reach the commit tstamp.

Thats not supposed to cause a gc?

All the sync gc functions are supposed to check against the reference
timestamp taken at the beginning of the transaction start, in
nf_tables_valid_genid().

They are NOT supposed to check against wall clock time.
Even if transaction takes 5 minutes: the sync gc step should
NOT observe even a single timed out elemenent.

They become stale only AFTER the sync gc step.

tstamp is coming from nft_nat->tstamp, i.e. start of transaction.
e->ext timestamp therefore should never be in the past compared
to that reference time.

1751                 if (__nft_set_elem_expired(&e->ext, tstamp)) {
1752                         if (!nft_trans_gc_space(gc)) {
1753                                 gc = nft_trans_gc_alloc(set, 0, GFP_KERNEL);
1754                                 if (!gc)

> I confirmed it on an unpatched 7.3-rc6 + KASAN: add 20000 elements with
> "timeout 10ms" to an interval+timeout pipapo set (the parse alone takes
> ~6s) and the set ends up basically empty. pipapo_gc_scan() collected the
> new, not-yet-activated elements during the same commit that added them.

How? I'm not seeing it :-(

> It's the same thing you already fixed for rbtree in 86b6471e690c, the

I thought that was about false zap of rbtree end nodes which doesn't
apply here :-(

> races the grace period just like the rbtree case, but the precondition
> (gc picking up the not-yet-active elements) reproduces every time.

How?

> If it'd help, I can respin v2 with the parse-time vs commit-time
> expiration written out in the commit message, just let me know.

Yes, please explain in detail how time can advance during transaction,
because its supposedly impossible.