Hi Nick,
Good catch and thanks for carefully reviewing this patch. Comments in-line.
On 6/22/2011 11:37 AM, Nicholas A. Bellinger wrote:
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.
Agree. Fix in progress and shall submit fixed patch soon to address
first comment.
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
Thanks,
-- Kiran P.
--
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
