On Mon, 2011-06-20 at 16:59 -0700, Robert Love wrote:
> From: Kiran Patil <kiran.patil@xxxxxxxxx>
>
> This patch fixes a bug in ft_send_tm() that was incorrectly calling
> ft_get_lun_for_cmd() -> transport_get_lun_for_cmd(), instead of using
> transport_get_lun_for_tmr() for the proper struct se_lun lookup.
>
> It also drops the now unnecessary ft_get_lun_for_cmd() code, and uses
> scsilun_to_int() directly ahead of direct transport_get_lun_for_cmd()
> and transport_get_lun_for_tmr usage().
>
> Patch was reworked due to NULL pointer access in function transport_get_lun_for_tmr(),
> (NOTE: transport_get_lun_for_tmr access tmr_req pointer which caused
> panic due to NULL pointer access) This patch fixes the issue by re-arranging
> the codepath where function "transport_get_lun_for_tmr" is called
> after tmr request is allocated and made it available as part of se_cmd.
>
> Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> Signed-off-by: Kiran Patil <kiran.patil@xxxxxxxxx>
> Signed-off-by: Robert Love <robert.w.love@xxxxxxxxx>
Hi Robert,
Thanks for the follow up on this. One comment below..
> ---
> drivers/target/tcm_fc/tcm_fc.h | 2 +
> drivers/target/tcm_fc/tfc_cmd.c | 62 ++++++++++++++++++++-------------------
> 2 files changed, 32 insertions(+), 32 deletions(-)
>
<SNIP>
> static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
> {
> struct se_queue_obj *qobj;
> @@ -426,13 +403,6 @@ static void ft_send_tm(struct ft_cmd *cmd)
> switch (fcp->fc_tm_flags) {
> case FCP_TMF_LUN_RESET:
> tm_func = TMR_LUN_RESET;
> - if (ft_get_lun_for_cmd(cmd, fcp->fc_lun) < 0) {
> - ft_dump_cmd(cmd, __func__);
> - transport_send_check_condition_and_sense(&cmd->se_cmd,
> - cmd->se_cmd.scsi_sense_reason, 0);
> - ft_sess_put(cmd->sess);
> - return;
> - }
> break;
> case FCP_TMF_TGT_RESET:
> tm_func = TMR_TARGET_WARM_RESET;
> @@ -464,6 +434,35 @@ static void ft_send_tm(struct ft_cmd *cmd)
> return;
> }
> cmd->se_cmd.se_tmr_req = tmr;
> +
> + switch (fcp->fc_tm_flags) {
> + case FCP_TMF_LUN_RESET:
> + cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
> + if (transport_get_lun_for_tmr(&cmd->se_cmd, cmd->lun) < 0) {
> + /*
> + * Make sure to clean up newly allocated TMR request
> + * since "unable to handle TMR request because failed
> + * to get to LUN"
> + */
> + FT_TM_DBG("Failed to get LUN for TMR func %d, "
> + "se_cmd %p, unpacked_lun %d\n",
> + tm_func, &cmd->se_cmd, cmd->lun);
> + transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
> + ft_dump_cmd(cmd, __func__);
> + transport_send_check_condition_and_sense(&cmd->se_cmd,
> + cmd->se_cmd.scsi_sense_reason, 0);
> + ft_sess_put(cmd->sess);
> + return;
This appears to be incorrect as transport_generic_free_cmd() will
release cmd->se_cmd before transport_send_check_condition_and_sense() is
called. I think they need to be reversed along the lines of how
ft_send_cmd() failure cases for -ENOMEM and -EINVAL are handled.
Here is a patch for lio-core-2.6.git with TCM v4.1 code to address the
issue, please review and I will plan to respin against mainline and send
off to Linus for -rc5.
Thanks!
--nab
------------------------------------------------------------------------------
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 6333170..1f2e9f5 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -390,6 +390,7 @@ static void ft_send_tm(struct ft_cmd *cmd)
{
struct se_tmr_req *tmr;
struct fcp_cmnd *fcp;
+ struct ft_sess *sess;
u8 tm_func;
fcp = fc_frame_payload_get(cmd->req_frame, sizeof(*fcp));
@@ -399,10 +400,20 @@ static void ft_send_tm(struct ft_cmd *cmd)
tm_func = TMR_LUN_RESET;
cmd->lun = scsilun_to_int((struct scsi_lun *)fcp->fc_lun);
if (transport_lookup_tmr_lun(&cmd->se_cmd, cmd->lun) < 0) {
+ /*
+ * Make sure to clean up newly allocated TMR request
+ * since "unable to handle TMR request because failed
+ * to get to LUN"
+ */
+ FT_TM_DBG("Failed to get LUN for TMR func %d, "
+ "se_cmd %p, unpacked_lun %d\n",
+ tm_func, &cmd->se_cmd, cmd->lun);
ft_dump_cmd(cmd, __func__);
+ sess = cmd->sess;
transport_send_check_condition_and_sense(&cmd->se_cmd,
cmd->se_cmd.scsi_sense_reason, 0);
- ft_sess_put(cmd->sess);
+ transport_generic_free_cmd(&cmd->se_cmd, 0, 0);
+ ft_sess_put(sess);
return;
}
break;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
