On Sun, Apr 09, 2017 at 12:21:22PM +0800, Liping Zhang wrote: > Hi Pablo, > > 2017-04-09 5:16 GMT+08:00 Pablo Neira Ayuso <[email protected]>: > > On Sat, Apr 01, 2017 at 10:14:24PM +0800, Liping Zhang wrote: > >> @@ -1960,9 +1955,7 @@ static int ctnetlink_new_conntrack(struct net *net, > >> struct sock *ctnl, > >> err = -EEXIST; > >> ct = nf_ct_tuplehash_to_ctrack(h); > >> if (!(nlh->nlmsg_flags & NLM_F_EXCL)) { > >> - spin_lock_bh(&nf_conntrack_expect_lock); > >> err = ctnetlink_change_conntrack(ct, cda); > >> - spin_unlock_bh(&nf_conntrack_expect_lock); > > > > We used to have a central spinlock here. > > > > spin_lock_bh(&nf_conntrack_lock); > > > > that was removed time ago, so this go converted to use > > nf_conntrack_expect_lock. > > This patch should add: > > Fixes: ca7433df3a67 ("netfilter: conntrack: seperate expect locking > from nf_conntrack_lock") > > Commit ca7433df3a67 add spin_lock_bh(&nf_conntrack_expect_lock) in > nf_ct_remove_expectations, but we also lock the _expect_lock before calling > ctnetlink_change_conntrack, so dead lock will happen: > > spin_lock_bh(&nf_conntrack_expect_lock): > ->err = ctnetlink_change_conntrack(ct, cda) > -->ctnetlink_change_helper > --->if (!strcmp(helpname, "")) nf_ct_remove_expectations() > ---->spin_lock_bh(&nf_conntrack_expect_lock); //lock _expect_lock > again, dead lock!
I agree this is fixing the deadlock but see below. > Since ctnetlink_change_conntrack is unrelated to nf_conntrack_expect_lock, > so remove it can fix this issue. But packets may be updating a conntrack at the same time that we're mangling via ctnetlink, right? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
