Missing rcu_read_unlock in glb-redirect? about glb-director HOT 5 CLOSED

Bump, do you have thoughts on this?

A colleague pointed out that nf_conntrack_find_get already does an rcu lock / unlock, so maybe the lock / unlock is not needed at this point in the first place?

from glb-director.

👍 This does look like an oversight, nice catch.

Looking back at history in an internal repo, the lock was added early on because conntrack would leak connections without it, I don't remember the specific context.

It does look like nf_conntrack_find_get takes a read lock around internal operations but I believe the lock needs to be held for the remaining usage of thash in the glb scope as well since the internal structure is still referenced.

from glb-director.

Thanks for raising the PR!

I spent some more time thinking about this, and I'm a bit puzzled about it's effects. From my understanding this should lead to machines deadlocking, since the rcu critical section is never left. If that is true, I would have expected you guys to experience & find this bug earlier.

After digging through the Linux source, I think it might be the case that the rcu_lock is never actually hit. See the comment on inet_ehash_bucket and inet_hashinfo. This would mean that SYN_RECV is always handled by the inet_lookup_established case, and conntrack would never find a tuple. What am I missing?

from glb-director.

From my understanding this should lead to machines deadlocking, since the rcu critical section is never left. If that is true, I would have expected you guys to experience & find this bug earlier.

I think it would if that code path was hit, but it looks like nf_ct_get_tuplepr already checks that the tuple nf_conntrack_find_get searches for exists, and then nf_ct_tuplehash_to_ctrack appears to always return a value in this scenario (every other use in nf_conntrack_core.c appears to just assume it returns a non-NULL value). There could be race conditions there, especially between the nf_ct_get_tuplepr and nf_conntrack_find_get, so it should definitely be released, but that's likely the reason we haven't observed it.

See the comment on inet_ehash_bucket and inet_hashinfo. This would mean that SYN_RECV is always handled by the inet_lookup_established case, and conntrack would never find a tuple. What am I missing?

IIRC, at least at the time this code was written, TCP sockets created on receipt of a SYN were not full sockets and weren't available in the established list (this commit mentions something about this torvalds/linux@10feb42). In that case, they were available in conntrack but the real socket was in the parent LISTEN socket which required a spinlock to be held in 3.x kernels.

It'd definitely be worth revisiting whether this is the case in modern (4.x series) kernels since lockless LISTEN access was added, I imagine a lot of this has changed, or at the very least we should be able to retrieve it all locklessly through the socket hashtables.

from glb-director.

IIRC, at least at the time this code was written, TCP sockets created on receipt of a SYN were not full sockets and weren't available in the established list (this commit mentions something about this torvalds/linux@10feb42). In that case, they were available in conntrack but the real socket was in the parent LISTEN socket which required a spinlock to be held in 3.x kernels.

Ah, that makes sense. My Linux network stack knowledge isn't great admittedly.

Thanks for fixing the RCU issue.

from glb-director.