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

[Joas Antonio]

Hi Florian,

You are right, and I was wrong. Sorry for the noise.

nft_set_elem_init() does stamp the expiration at parse time:

    nf_tables_api.c:6860
    nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;

but I misread where the reference timestamp comes from. It is taken
in nf_tables_valid_genid(), before the first message of the batch is
processed, not in nf_tables_commit(). So expiration is always at least
tstamp + timeout, and the sync gc in pipapo_gc_scan() cannot see a
new element as expired, no matter how long the transaction takes.

The "empty set" I observed is explained by the elements simply timing
out after the commit (10ms timeout), and the dump skipping expired
elements. It was not the gc collecting inactive elements. The
comparison with 86b6471e690c was also wrong.

Please drop the patch. I'm withdrawing it.

Thanks for taking the time to check it.
Joas


Em dom., 25 de out. de 2026 às 02:38, Florian Westphal <fw@xxxxxxxxx> escreveu:
>
> 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.



-- 
Joas Antonio dos Santos
Cyber Security Specialist and Ethical Hacker
joasantonio108@xxxxxxxxx
LinkedIn