Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751314AbdFAGVh (ORCPT ); Thu, 1 Jun 2017 02:21:37 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:35670 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750848AbdFAGVf (ORCPT ); Thu, 1 Jun 2017 02:21:35 -0400 Message-ID: <1496298093.27407.203.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH] iscsi: Fix a sleep-in-atomic bug From: "Nicholas A. Bellinger" To: Jia-Ju Bai Cc: bart.vanassche@sandisk.com, davem@davemloft.net, hare@suse.com, elfring@users.sourceforge.net, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 31 May 2017 23:21:33 -0700 In-Reply-To: <1496201199-3661-1-git-send-email-baijiaju1990@163.com> References: <1496201199-3661-1-git-send-email-baijiaju1990@163.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2488 Lines: 66 Hi Jia-Ju, On Wed, 2017-05-31 at 11:26 +0800, Jia-Ju Bai wrote: > The driver may sleep under a spin lock, and the function call path is: > iscsit_tpg_enable_portal_group (acquire the lock by spin_lock) > iscsi_update_param_value > kstrdup(GFP_KERNEL) --> may sleep > > To fix it, the "GFP_KERNEL" is replaced with "GFP_ATOMIC". > > Signed-off-by: Jia-Ju Bai > --- > drivers/target/iscsi/iscsi_target_parameters.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Btw, the use of tpg->tpg_state_lock in iscsit_tpg_enable_portal_group() while checking existing state and calling iscsi_update_param_value() is not necessary, since lio_target_tpg_enable_store() is already holding iscsit_get_tpg() -> tpg->tpg_access_lock. How about the following instead to only take tpg->tpg_state_lock when updating tpg->tpg_state instead..? diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c index 2e7e08d..abaabba 100644 --- a/drivers/target/iscsi/iscsi_target_tpg.c +++ b/drivers/target/iscsi/iscsi_target_tpg.c @@ -311,11 +311,9 @@ int iscsit_tpg_enable_portal_group(struct iscsi_portal_group *tpg) struct iscsi_tiqn *tiqn = tpg->tpg_tiqn; int ret; - spin_lock(&tpg->tpg_state_lock); if (tpg->tpg_state == TPG_STATE_ACTIVE) { pr_err("iSCSI target portal group: %hu is already" " active, ignoring request.\n", tpg->tpgt); - spin_unlock(&tpg->tpg_state_lock); return -EINVAL; } /* @@ -324,10 +322,8 @@ int iscsit_tpg_enable_portal_group(struct iscsi_portal_group *tpg) * is enforced (as per default), and remove the NONE option. */ param = iscsi_find_param_from_key(AUTHMETHOD, tpg->param_list); - if (!param) { - spin_unlock(&tpg->tpg_state_lock); + if (!param) return -EINVAL; - } if (tpg->tpg_attrib.authentication) { if (!strcmp(param->value, NONE)) { @@ -341,6 +337,7 @@ int iscsit_tpg_enable_portal_group(struct iscsi_portal_group *tpg) goto err; } + spin_lock(&tpg->tpg_state_lock); tpg->tpg_state = TPG_STATE_ACTIVE; spin_unlock(&tpg->tpg_state_lock); @@ -353,7 +350,6 @@ int iscsit_tpg_enable_portal_group(struct iscsi_portal_group *tpg) return 0; err: - spin_unlock(&tpg->tpg_state_lock); return ret; }