Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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.
