Pablo Neira Ayuso <[email protected]> wrote:
> 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 <[email protected]>:
> > [...]
> > > 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?
No objection, thanks!
> I will append the Fixes: tag:
>
> Fixes: 89f2e21883b5 ("[NETFILTER]: ctnetlink: change table dumping not to
> require an unique ID")
That commit looks fine to me, it seems to make sure to put
"last" only once in all cases.
93bb0ceb75be2fdfa9fc0dd1 however adds a check on cb->args[0], and if
that is hit it will do a put() on last, and then, the "done" netlink
callback will do another put operation on cb->args[1] (i.e., last).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
