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 <pablo@xxxxxxxxxxxxx>:
> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
