Re: [PATCH nf 1/2] netfilter: ctnetlink: fix refcount leak on table dump

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Aug 01, 2025 at 05:25:08PM +0200, Florian Westphal wrote:
> > There is a reference count leak in ctnetlink_dump_table():
> >       if (res < 0) {
> >                 nf_conntrack_get(&ct->ct_general); // HERE
> >                 cb->args[1] = (unsigned long)ct;
> >                 ...
>                   goto out;
> 
> >
> > While its very unlikely, its possible that ct == last.
> 
> out:
>         ...
>         if (last) {
>                 /* nf ct hash resize happened, now clear the leftover. */
>                 if ((struct nf_conn *)cb->args[1] == last) {
>                         cb->args[1] = 0;
>                 }
> 
>                 nf_ct_put(last);
>         }
> 
> I think problem was introduced here:
> 
>   fefa92679dbe ("netfilter: ctnetlink: fix incorrect nf_ct_put during hash resize")

I think you'r right, the 'clear the leftover' is only correct if
we hit cb->args[0] >= htable_size condition.
OTOH reverting it gives the problem that commit fixed.

So I think that this code is just way too complicated,
i have no idea why this ever used reference counts, they do not buy
anything but headaches.

> cookie is indeed safer approach.
> 
> IIRC, the concern is that cookie could result in providing a bogus
> conntrack listing due to object recycling, which is more likely to
> happen with SLAB_TYPESAFE_BY_RCU.

Maybe, but even if this code would just store the address, the probability
of a recycle happening in such a way that a conntrack oject happens to be
stored, and then on next dump got re-added at exactly this slot is
almost 0.

And even if it would have been, the worst that can happen is that we
dump another entry a second time.  /proc code uses to walk the entire
table from start, counting dumped-entries and I'm not aware of 'dup'
complaints.

> Then, it should be very unlikely that such recycling that leads to
> picking up from the wrong conntrack object because two conntrack
> objects in the same memory spot will have different id.

Yes, it considers the tuples for the hash too, so its exteremly
unlikely for a recycle to result in same u32 hash value.