3.16.47-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Liping Zhang <zlpnobody@xxxxxxxxx>
commit 88be4c09d9008f9ff337cbf48c5d0f06c8f872e7 upstream.
Currently, ctnetlink_change_conntrack is always protected by _expect_lock,
but this will cause a deadlock when deleting the helper from a conntrack,
as the _expect_lock will be acquired again by nf_ct_remove_expectations:
CPU0
----
lock(nf_conntrack_expect_lock);
lock(nf_conntrack_expect_lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
2 locks held by lt-conntrack_gr/12853:
#0: (&table[i].mutex){+.+.+.}, at: [<ffffffffa05e2009>]
nfnetlink_rcv_msg+0x399/0x6a9 [nfnetlink]
#1: (nf_conntrack_expect_lock){+.....}, at: [<ffffffffa05f2c1f>]
ctnetlink_new_conntrack+0x17f/0x408 [nf_conntrack_netlink]
Call Trace:
dump_stack+0x85/0xc2
__lock_acquire+0x1608/0x1680
? ctnetlink_parse_tuple_proto+0x10f/0x1c0 [nf_conntrack_netlink]
lock_acquire+0x100/0x1f0
? nf_ct_remove_expectations+0x32/0x90 [nf_conntrack]
_raw_spin_lock_bh+0x3f/0x50
? nf_ct_remove_expectations+0x32/0x90 [nf_conntrack]
nf_ct_remove_expectations+0x32/0x90 [nf_conntrack]
ctnetlink_change_helper+0xc6/0x190 [nf_conntrack_netlink]
ctnetlink_new_conntrack+0x1b2/0x408 [nf_conntrack_netlink]
nfnetlink_rcv_msg+0x60a/0x6a9 [nfnetlink]
? nfnetlink_rcv_msg+0x1b9/0x6a9 [nfnetlink]
? nfnetlink_bind+0x1a0/0x1a0 [nfnetlink]
netlink_rcv_skb+0xa4/0xc0
nfnetlink_rcv+0x87/0x770 [nfnetlink]
Since the operations are unrelated to nf_ct_expect, so we can drop the
_expect_lock. Also note, after removing the _expect_lock protection,
another CPU may invoke nf_conntrack_helper_unregister, so we should
use rcu_read_lock to protect __nf_conntrack_helper_find invoked by
ctnetlink_change_helper.
Fixes: ca7433df3a67 ("netfilter: conntrack: seperate expect locking from nf_conntrack_lock")
Signed-off-by: Liping Zhang <zlpnobody@xxxxxxxxx>
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
[bwh: Backported to 3.16:
- ctnetlink_change_helper() still auto-loads modules, so update the unlocking
and re-locking there
- Adjust context]
Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1384,24 +1384,22 @@ ctnetlink_change_helper(struct nf_conn *
return 0;
}
+ rcu_read_lock();
helper = __nf_conntrack_helper_find(helpname, nf_ct_l3num(ct),
nf_ct_protonum(ct));
if (helper == NULL) {
#ifdef CONFIG_MODULES
- spin_unlock_bh(&nf_conntrack_expect_lock);
+ rcu_read_unlock();
- if (request_module("nfct-helper-%s", helpname) < 0) {
- spin_lock_bh(&nf_conntrack_expect_lock);
+ if (request_module("nfct-helper-%s", helpname) < 0)
return -EOPNOTSUPP;
- }
- spin_lock_bh(&nf_conntrack_expect_lock);
+ rcu_read_lock();
helper = __nf_conntrack_helper_find(helpname, nf_ct_l3num(ct),
nf_ct_protonum(ct));
- if (helper)
- return -EAGAIN;
#endif
- return -EOPNOTSUPP;
+ rcu_read_unlock();
+ return helper ? -EAGAIN : -EOPNOTSUPP;
}
if (help) {
@@ -1409,13 +1407,16 @@ ctnetlink_change_helper(struct nf_conn *
/* update private helper data if allowed. */
if (helper->from_nlattr)
helper->from_nlattr(helpinfo, ct);
- return 0;
+ err = 0;
} else
- return -EBUSY;
+ err = -EBUSY;
+ } else {
+ /* we cannot set a helper for an existing conntrack */
+ err = -EOPNOTSUPP;
}
- /* we cannot set a helper for an existing conntrack */
- return -EOPNOTSUPP;
+ rcu_read_unlock();
+ return err;
}
static inline int
@@ -1831,9 +1832,7 @@ ctnetlink_new_conntrack(struct sock *ctn
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);
if (err == 0) {
nf_conntrack_eventmask_report((1 << IPCT_REPLY) |
(1 << IPCT_ASSURED) |
@@ -2165,11 +2164,7 @@ ctnetlink_nfqueue_parse(const struct nla
if (ret < 0)
return ret;
- spin_lock_bh(&nf_conntrack_expect_lock);
- ret = ctnetlink_nfqueue_parse_ct((const struct nlattr **)cda, ct);
- spin_unlock_bh(&nf_conntrack_expect_lock);
-
- return ret;
+ return ctnetlink_nfqueue_parse_ct((const struct nlattr **)cda, ct);
}
static int ctnetlink_nfqueue_exp_parse(const struct nlattr * const *cda,