From patchwork Mon Nov 28 11:22:12 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 699937 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3tS46M0mB5z9sDB for ; Mon, 28 Nov 2016 22:22:51 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754289AbcK1LWr (ORCPT ); Mon, 28 Nov 2016 06:22:47 -0500 Received: from helcar.hengli.com.au ([209.40.204.226]:48989 "EHLO helcar.apana.org.au" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753929AbcK1LWo (ORCPT ); Mon, 28 Nov 2016 06:22:44 -0500 Received: from [192.168.128.4] (helo=gondobar) by fornost.hengli.com.au with esmtp (Exim 4.80 #3 (Debian)) id 1cBK0u-0003aI-FS; Mon, 28 Nov 2016 22:22:30 +1100 Received: from herbert by gondobar with local (Exim 4.84_2) (envelope-from ) id 1cBK0i-0000S2-9l; Mon, 28 Nov 2016 19:22:12 +0800 Date: Mon, 28 Nov 2016 19:22:12 +0800 From: Herbert Xu To: Cong Wang Cc: Eric Dumazet , Subash Abhinov Kasiviswanathan , Thomas Graf , Linux Kernel Network Developers Subject: Re: Crash due to mutex genl_lock called from RCU context Message-ID: <20161128112211.GA990@gondor.apana.org.au> References: <1480133493.8455.584.camel@edumazet-glaptop3.roam.corp.google.com> <1480136078.8455.589.camel@edumazet-glaptop3.roam.corp.google.com> <1480213570.18162.31.camel@edumazet-glaptop3.roam.corp.google.com> <1480263824.18162.44.camel@edumazet-glaptop3.roam.corp.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sun, Nov 27, 2016 at 10:53:21PM -0800, Cong Wang wrote: > > I just took a deeper look, some user calls rhashtable_destroy() in ->done(), > so even removing that genl lock is not enough, perhaps we should just > move it to a work struct like what Daniel does for the tcf_proto, but that is > ugly... I don't know if RCU provides any API to execute the callback in process > context. I looked into doing it without a worker struct, but basically it means that we'd have to add more crap to the common code path for what is essentially a very rare case. So I think we should go with the worker struct because all we lose is a bit of memory. ---8<--- netlink: Call cb->done from a worker thread The cb->done interface expects to be called in process context. This was broken by the netlink RCU conversion. This patch fixes it by adding a worker struct to make the cb->done call where necessary. Fixes: 21e4902aea80 ("netlink: Lockless lookup with RCU grace...") Reported-by: Subash Abhinov Kasiviswanathan Signed-off-by: Herbert Xu Acked-by: Cong Wang diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 62bea45..602e5eb 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -322,14 +322,11 @@ static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk) sk_mem_charge(sk, skb->truesize); } -static void netlink_sock_destruct(struct sock *sk) +static void __netlink_sock_destruct(struct sock *sk) { struct netlink_sock *nlk = nlk_sk(sk); if (nlk->cb_running) { - if (nlk->cb.done) - nlk->cb.done(&nlk->cb); - module_put(nlk->cb.module); kfree_skb(nlk->cb.skb); } @@ -346,6 +343,28 @@ static void netlink_sock_destruct(struct sock *sk) WARN_ON(nlk_sk(sk)->groups); } +static void netlink_sock_destruct_work(struct work_struct *work) +{ + struct netlink_sock *nlk = container_of(work, struct netlink_sock, + work); + + nlk->cb.done(&nlk->cb); + __netlink_sock_destruct(&nlk->sk); +} + +static void netlink_sock_destruct(struct sock *sk) +{ + struct netlink_sock *nlk = nlk_sk(sk); + + if (nlk->cb_running && nlk->cb.done) { + INIT_WORK(&nlk->work, netlink_sock_destruct_work); + schedule_work(&nlk->work); + return; + } + + __netlink_sock_destruct(sk); +} + /* This lock without WQ_FLAG_EXCLUSIVE is good on UP and it is _very_ bad on * SMP. Look, when several writers sleep and reader wakes them up, all but one * immediately hit write lock and grab all the cpus. Exclusive sleep solves diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h index 3cfd6cc..4fdb383 100644 --- a/net/netlink/af_netlink.h +++ b/net/netlink/af_netlink.h @@ -3,6 +3,7 @@ #include #include +#include #include #define NLGRPSZ(x) (ALIGN(x, sizeof(unsigned long) * 8) / 8) @@ -33,6 +34,7 @@ struct netlink_sock { struct rhash_head node; struct rcu_head rcu; + struct work_struct work; }; static inline struct netlink_sock *nlk_sk(struct sock *sk)