| Message ID | 1496298093.27407.203.camel@haakon3.risingtidesystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }