From patchwork Mon Apr 17 13:18:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Liping Zhang X-Patchwork-Id: 751310 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3w684n5M6Sz9ryk for ; Mon, 17 Apr 2017 23:19:53 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=163.com header.i=@163.com header.b="EOBNJrYx"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753695AbdDQNTv (ORCPT ); Mon, 17 Apr 2017 09:19:51 -0400 Received: from m12-17.163.com ([220.181.12.17]:56527 "EHLO m12-17.163.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753354AbdDQNTt (ORCPT ); Mon, 17 Apr 2017 09:19:49 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=From:Subject:Date:Message-Id; bh=HtiAd4u3/pasgbb/8v GoCiGc5ZEJQBpthVaUG2+iffc=; b=EOBNJrYxFo0hSBCI3ghAYe9N6DLedRT8Ny u92By3c+Ine9Z/5tvuEmz1NdOALkLlHQWpSGzPVJuTljL9qL57rkfwCcKbhUpdxP GU34WBga2nxWxZBJTXhtagP0+w1PfWYekhqwi8NRqX1VsArDgR2roZam5sZqaFHl SPB7sq9k0= Received: from MiWiFi-R2D-srv.localdomain (unknown [180.164.231.180]) by smtp13 (Coremail) with SMTP id EcCowACXs8PewPRY6P6cAg--.35942S4; Mon, 17 Apr 2017 21:19:42 +0800 (CST) From: Liping Zhang To: pablo@netfilter.org Cc: netfilter-devel@vger.kernel.org, Liping Zhang Subject: [PATCH nf 2/4] netfilter: ctnetlink: fix deadlock due to acquire _expect_lock twice Date: Mon, 17 Apr 2017 21:18:56 +0800 Message-Id: <1492435138-28283-3-git-send-email-zlpnobody@163.com> X-Mailer: git-send-email 2.5.5 In-Reply-To: <1492435138-28283-1-git-send-email-zlpnobody@163.com> References: <1492435138-28283-1-git-send-email-zlpnobody@163.com> X-CM-TRANSID: EcCowACXs8PewPRY6P6cAg--.35942S4 X-Coremail-Antispam: 1Uf129KBjvJXoWxAFWxAFW7GFyUuF45KF1kuFg_yoWrXw1rp3 yfKryfJr17Ja1Fv3W7Ka4qva12qan3ua1rWrySkFZ5CF15trWak3y8trsxur9Iyrs7try7 AF429r15AF1rA3JanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07jXEfOUUUUU= X-Originating-IP: [180.164.231.180] X-CM-SenderInfo: x2os00perg5qqrwthudrp/1tbiQA28l1SISrBOaQAAsn Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org From: Liping Zhang 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: [] nfnetlink_rcv_msg+0x399/0x6a9 [nfnetlink] #1: (nf_conntrack_expect_lock){+.....}, at: [] 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 --- net/netfilter/nf_conntrack_netlink.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 48c1845..e5f9777 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1510,23 +1510,29 @@ static int ctnetlink_change_helper(struct nf_conn *ct, return 0; } + rcu_read_lock(); helper = __nf_conntrack_helper_find(helpname, nf_ct_l3num(ct), nf_ct_protonum(ct)); - if (helper == NULL) + if (helper == NULL) { + rcu_read_unlock(); return -EOPNOTSUPP; + } if (help) { if (help->helper == helper) { /* 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 int ctnetlink_change_timeout(struct nf_conn *ct, @@ -1945,9 +1951,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); if (err == 0) { nf_conntrack_eventmask_report((1 << IPCT_REPLY) | (1 << IPCT_ASSURED) | @@ -2342,11 +2346,7 @@ ctnetlink_glue_parse(const struct nlattr *attr, struct nf_conn *ct) if (ret < 0) return ret; - spin_lock_bh(&nf_conntrack_expect_lock); - ret = ctnetlink_glue_parse_ct((const struct nlattr **)cda, ct); - spin_unlock_bh(&nf_conntrack_expect_lock); - - return ret; + return ctnetlink_glue_parse_ct((const struct nlattr **)cda, ct); } static int ctnetlink_glue_exp_parse(const struct nlattr * const *cda,