Re: [PATCH nf] netfilter: ctnetlink: fix incorrect nf_ct_put during hash resize

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

On Sun, May 21, 2017 at 08:59:45AM +0800, Liping Zhang wrote:
> Hi Florian,
> 
> 2017-05-21 8:00 GMT+08:00 Florian Westphal <fw@strlen.de>:
> [...]
> > Yes, you're right, seems this was added in
> > 93bb0ceb75be2fdfa9fc0dd1fb522d9ada515d9c (it adds the 'goto out').
> 
> I added some trace logs, and when the hash size reduced, for example,
> from 60000 to 500, then the issue would happen.
> 
> Actually, hitting 'goto out' is not easy, so the issue exists for a very long
> time. Maybe commit 89f2e21883b5("[NETFILTER]: ctnetlink: change
> table dumping not to require an unique ID") is to blame for it.
> 
> > Your patch looks correct.
> >
> > However, why do we bump refcnt of 'last' in the first place?
> >
> > Its only the continuation marker, i.e. its expected to reside
> > in the hash slot at cb->args[0], but after rehash this might not
> > be true either.
> >
> > I think we should simplify this, just take the verbatim address,
> > and clear it right at start of ctnetlink_dump_table, i.e.
> >
> > unsigned long last = cb->args[1];
> > cb->args[1] = 0;
> >
> > for (; cb->args[0] < nf_conntrack_htable_size; cb->args[0]++) {
> >          ...
> >     hlist_nulls_for_each_entry ... {
> >        ...
> >        if (last) {
> >             if (last != (unsigned long)ct))
> >                cont;
> >             last = 0;
> >        }
> >        ...
> >        dump();
> >     }
> >     last = 0; /* reset it, as it wasn't in args[0] slot */
> >  }
> >
> > Do you see any problem with that?
> 
> I think this will be better, this will make code more clean.
> Also we can clean up the ctnetlink_exp_ct_dump_table too.

@Florian, no objection then if I place this into nf.git?

I will append the Fixes: tag:

Fixes: 89f2e21883b5 ("[NETFILTER]: ctnetlink: change table dumping not to require an unique ID")