Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> On Fri, 2016-08-19 at 17:16 +0200, Florian Westphal wrote:
>
> > Hmm, ____nf_conntrack_find caller needs to hold rcu_read_lock,
> > in case object is free'd SLAB_DESTROY_BY_RCU should delay actual release
> > of the page.
>
> Well, point is that SLAB_DESTROY_BY_RCU means that we have no grace
> period, and object can be immediately reused and recycled.
>
> @next pointer can definitely be overwritten.
I see. Isn't that detected by the nulls magic (to restart
lookup if entry was moved to other chain due to overwritten next pointer)?
I don't see a hlist_nulls_for_each_entry_rcu_safe variant like we have
for normal lists (list_for_each_entry_safe), do I need to add one?
Currently we have this:
rcu_read_lock()
____nf_conntrack_find
hlist_nulls_for_each_entry_rcu ...
if (nf_ct_is_expired(ct)) { // lazy test, ct can be reused here
nf_ct_gc_expired(ct);
continue;
}
nf_ct_gc_expired() does this:
static void nf_ct_gc_expired(struct nf_conn *ct)
{
if (!atomic_inc_not_zero(&ct->ct_general.use))
return;
if (nf_ct_should_gc(ct))
nf_ct_kill(ct);
nf_ct_put(ct);
So we only nf_ct_kill when we have a reference
and we know ct is in hash (check is in nf_ct_should_gc()).
So once we put, the object can be released but the
next pointer is not altered.
In case ct object is reused right after this last
nf_ct_put it could be re-inserted already, e.g. into
dying list or back into hash table -- but is this a problem?
'dead' (non-recycled) element is cought by atomic_inc_not_zero
in nf_ct_gc_expired, about-to-inserted (not in hash) is detected
by nf_ct_should_gc() (it checks confirmed bit in ct->status),
already-inserted (in hashtable) would lead us to 'magically'
move to a different hash chain in hlist_nulls_for_each_entry_rcu.
But we would then hit the wrong nulls list terminator and restart
the traversal.
Does that make sense?
--
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
