Re: [PATCH nf] netfilter: ctnetlink: remove unnecessary nf_conntrack_expect_lock protection

[Pablo Neira Ayuso <pablo@netfilter.org>]

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@netfilter.org>:
> > 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?