Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> +static struct nf_conn *get_next_tmpl(struct net *net)
> +{
> + struct nf_conntrack_tuple_hash *h;
> + struct hlist_nulls_node *n;
> + struct nf_conn *ct = NULL;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct ct_pcpu *pcpu = per_cpu_ptr(net->ct.pcpu_lists, cpu);
> +
> + spin_lock_bh(&pcpu->lock);
> + hlist_nulls_for_each_entry(h, n, &pcpu->tmpl, hnnode) {
> + ct = nf_ct_tuplehash_to_ctrack(h);
> + break;
> + }
> + spin_unlock_bh(&pcpu->lock);
> + }
> + return ct;
> +}
> +
> +void nf_ct_tmpl_cleanup(struct net *net)
> +{
> + struct nf_conn *tmpl;
> +
> +i_see_dead_people:
> + while ((tmpl = get_next_tmpl(net)) != NULL)
> + nf_ct_put(tmpl);
> +
This seems bogus, this _puts() conntracks we don't own, leading
to refcnt underflow/use-after-free?
I'd just get rid of all of the above. After your change we
don't depend on the kmem_cache anymore so its ok to leave freeing to
the CT target ->destroy() hook, provided we also fix up the tmpl list
handling (i'd suggest to just remove it and make each template sit
on their 'own' list -- so the hlist_nulls_unhashed() BUG_ONs don't
fire).
Other than that, this patch seems like the least ugly approach.
Thanks Pablo!
--
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
