* [PATCH] iscsi: Fix a sleep-in-atomic bug
@ 2017-05-31 3:26 Jia-Ju Bai
2017-06-01 6:21 ` Nicholas A. Bellinger
0 siblings, 1 reply; 4+ messages in thread
From: Jia-Ju Bai @ 2017-05-31 3:26 UTC (permalink / raw)
To: nab, bart.vanassche, davem, hare, elfring
Cc: linux-scsi, target-devel, linux-kernel, Jia-Ju Bai
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 <baijiaju1990@163.com>
---
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index fce6276..8768916 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -702,7 +702,7 @@ int iscsi_update_param_value(struct iscsi_param *param, char *value)
{
kfree(param->value);
- param->value = kstrdup(value, GFP_KERNEL);
+ param->value = kstrdup(value, GFP_ATOMIC);
if (!param->value) {
pr_err("Unable to allocate memory for value.\n");
return -ENOMEM;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] iscsi: Fix a sleep-in-atomic bug 2017-05-31 3:26 [PATCH] iscsi: Fix a sleep-in-atomic bug Jia-Ju Bai @ 2017-06-01 6:21 ` Nicholas A. Bellinger 2017-06-02 1:13 ` Jia-Ju Bai 0 siblings, 1 reply; 4+ messages in thread From: Nicholas A. Bellinger @ 2017-06-01 6:21 UTC (permalink / raw) To: Jia-Ju Bai Cc: bart.vanassche, davem, hare, elfring, linux-scsi, target-devel, linux-kernel 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 <baijiaju1990@163.com> > --- > 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; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iscsi: Fix a sleep-in-atomic bug 2017-06-01 6:21 ` Nicholas A. Bellinger @ 2017-06-02 1:13 ` Jia-Ju Bai 2017-06-02 3:20 ` Nicholas A. Bellinger 0 siblings, 1 reply; 4+ messages in thread From: Jia-Ju Bai @ 2017-06-02 1:13 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: bart.vanassche, davem, hare, elfring, linux-scsi, target-devel, linux-kernel On 06/01/2017 02:21 PM, Nicholas A. Bellinger wrote: > 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<baijiaju1990@163.com> >> --- >> 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; > } > I think it is fine to me. Thanks, Jia-Ju Bai ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iscsi: Fix a sleep-in-atomic bug 2017-06-02 1:13 ` Jia-Ju Bai @ 2017-06-02 3:20 ` Nicholas A. Bellinger 0 siblings, 0 replies; 4+ messages in thread From: Nicholas A. Bellinger @ 2017-06-02 3:20 UTC (permalink / raw) To: Jia-Ju Bai Cc: bart.vanassche, davem, hare, elfring, linux-scsi, target-devel, linux-kernel On Fri, 2017-06-02 at 09:13 +0800, Jia-Ju Bai wrote: > On 06/01/2017 02:21 PM, Nicholas A. Bellinger wrote: > > 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<baijiaju1990@163.com> > >> --- > >> 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; > > } > > > I think it is fine to me. > > Thanks, > Jia-Ju Bai Applied with your Reported-by and Reviewed-by. Thanks! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-02 3:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-31 3:26 [PATCH] iscsi: Fix a sleep-in-atomic bug Jia-Ju Bai 2017-06-01 6:21 ` Nicholas A. Bellinger 2017-06-02 1:13 ` Jia-Ju Bai 2017-06-02 3:20 ` Nicholas A. Bellinger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.