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

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

Hi Florian,

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")

> If this happens, then the refcount of ct was already incremented.
> This 2nd increment is never undone.
> 
> This prevents the conntrack object from being released, which in turn
> keeps prevents cnet->count from dropping back to 0.
> 
> This will then block the netns dismantle (or conntrack rmmod) as
> nf_conntrack_cleanup_net_list() will wait forever.
> 
> This can be reproduced by running conntrack_resize.sh selftest in a loop.
> It takes ~20 minutes for me on a preemptible kernel on average before
> I see a runaway kworker spinning in nf_conntrack_cleanup_net_list.
> 
> One fix would to change this to:
>         if (res < 0) {
> 		if (ct != last)
> 	                nf_conntrack_get(&ct->ct_general);
> 
> But this reference counting isn't needed in the first place.
> We can just store a cookie value instead.

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.

nf_ct_get_id() is adding using a random seed to generate the conntrack
id:

u32 nf_ct_get_id(const struct nf_conn *ct)
{
        static siphash_aligned_key_t ct_id_seed;
        unsigned long a, b, c, d;

        net_get_random_once(&ct_id_seed, sizeof(ct_id_seed));

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.