Commit 5195c14c authored by bill bonaparte's avatar bill bonaparte Committed by Pablo Neira Ayuso

netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

After removal of the central spinlock nf_conntrack_lock, in
commit 93bb0ceb ("netfilter: conntrack: remove central
spinlock nf_conntrack_lock"), it is possible to race against
get_next_corpse().

The race is against the get_next_corpse() cleanup on
the "unconfirmed" list (a per-cpu list with seperate locking),
which set the DYING bit.

Fix this race, in __nf_conntrack_confirm(), by removing the CT
from unconfirmed list before checking the DYING bit.  In case
race occured, re-add the CT to the dying list.

While at this, fix coding style of the comment that has been
updated.

Fixes: 93bb0ceb ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
Reported-by: default avatarbill bonaparte <programme110@gmail.com>
Signed-off-by: default avatarbill bonaparte <programme110@gmail.com>
Signed-off-by: default avatarJesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent b326dd37
...@@ -611,12 +611,16 @@ __nf_conntrack_confirm(struct sk_buff *skb) ...@@ -611,12 +611,16 @@ __nf_conntrack_confirm(struct sk_buff *skb)
*/ */
NF_CT_ASSERT(!nf_ct_is_confirmed(ct)); NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
pr_debug("Confirming conntrack %p\n", ct); pr_debug("Confirming conntrack %p\n", ct);
/* We have to check the DYING flag inside the lock to prevent
a race against nf_ct_get_next_corpse() possibly called from /* We have to check the DYING flag after unlink to prevent
user context, else we insert an already 'dead' hash, blocking * a race against nf_ct_get_next_corpse() possibly called from
further use of that particular connection -JM */ * user context, else we insert an already 'dead' hash, blocking
* further use of that particular connection -JM.
*/
nf_ct_del_from_dying_or_unconfirmed_list(ct);
if (unlikely(nf_ct_is_dying(ct))) { if (unlikely(nf_ct_is_dying(ct))) {
nf_ct_add_to_dying_list(ct);
nf_conntrack_double_unlock(hash, reply_hash); nf_conntrack_double_unlock(hash, reply_hash);
local_bh_enable(); local_bh_enable();
return NF_ACCEPT; return NF_ACCEPT;
...@@ -636,8 +640,6 @@ __nf_conntrack_confirm(struct sk_buff *skb) ...@@ -636,8 +640,6 @@ __nf_conntrack_confirm(struct sk_buff *skb)
zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)))
goto out; goto out;
nf_ct_del_from_dying_or_unconfirmed_list(ct);
/* Timer relative to confirmation time, not original /* Timer relative to confirmation time, not original
setting time, otherwise we'd get timer wrap in setting time, otherwise we'd get timer wrap in
weird delay cases. */ weird delay cases. */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment