Hi Florian,
2017-05-21 8:00 GMT+08:00 Florian Westphal <fw@xxxxxxxxx>:
[...]
> 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.
>
> [ It might be better to take your patch for nf- though and do
> this no-refcnt thing in nf-next ... ]
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
