* [PATCH v4 0/4] target: fix NULL pointer dereference @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma The following set of commits address a NULL pointer dereference and some refactoring around this issue. v4: - initialize the LUN in transport_init_se_cmd() v3: - fix NULL pointer dereference when cdb initialization fails v2: - new helper is named as target_cmd_init_cdb() - existing function, target_setup_cmd_from_cdb is renamed as target_cmd_parse_cdb() Sudhakar Panneerselvam (4): target: factor out a new helper, target_cmd_init_cdb() target: initialize LUN in transport_init_se_cmd(). target: fix NULL pointer dereference target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() drivers/target/iscsi/iscsi_target.c | 29 ++++++++++-------- drivers/target/target_core_device.c | 19 +++++------- drivers/target/target_core_tmr.c | 4 +-- drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++-------- drivers/target/target_core_xcopy.c | 9 ++++-- drivers/usb/gadget/function/f_tcm.c | 6 ++-- include/target/target_core_fabric.h | 9 +++--- 7 files changed, 83 insertions(+), 48 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 0/4] target: fix NULL pointer dereference @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma The following set of commits address a NULL pointer dereference and some refactoring around this issue. v4: - initialize the LUN in transport_init_se_cmd() v3: - fix NULL pointer dereference when cdb initialization fails v2: - new helper is named as target_cmd_init_cdb() - existing function, target_setup_cmd_from_cdb is renamed as target_cmd_parse_cdb() Sudhakar Panneerselvam (4): target: factor out a new helper, target_cmd_init_cdb() target: initialize LUN in transport_init_se_cmd(). target: fix NULL pointer dereference target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() drivers/target/iscsi/iscsi_target.c | 29 ++++++++++-------- drivers/target/target_core_device.c | 19 +++++------- drivers/target/target_core_tmr.c | 4 +-- drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++-------- drivers/target/target_core_xcopy.c | 9 ++++-- drivers/usb/gadget/function/f_tcm.c | 6 ++-- include/target/target_core_fabric.h | 9 +++--- 7 files changed, 83 insertions(+), 48 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 1/4] target: factor out a new helper, target_cmd_init_cdb() 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-07 19:58 ` Sudhakar Panneerselvam -1 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma target_setup_cmd_from_cdb() is called after a successful call to transport_lookup_cmd_lun(). The new helper factors out the code that can be called before the call to transport_lookup_cmd_lun(). This helper will be used in an upcoming commit to address NULL pointer dereference. Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/target_core_transport.c | 16 ++++++++++++---- include/target/target_core_fabric.h | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index e6b448f43071..f2f7c5b818cc 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1410,11 +1410,8 @@ void transport_init_se_cmd( } sense_reason_t -target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb) { - struct se_device *dev = cmd->se_dev; - sense_reason_t ret; - /* * Ensure that the received CDB is less than the max (252 + 8) bytes * for VARIABLE_LENGTH_CMD @@ -1448,6 +1445,17 @@ void transport_init_se_cmd( memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb)); trace_target_sequencer_start(cmd); + return 0; +} +EXPORT_SYMBOL(target_cmd_init_cdb); + +sense_reason_t +target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +{ + struct se_device *dev = cmd->se_dev; + sense_reason_t ret; + + target_cmd_init_cdb(cmd, cdb); ret = dev->transport->parse_cdb(cmd); if (ret = TCM_UNSUPPORTED_SCSI_OPCODE) diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 063f133e47c2..6a2bfcca0c98 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -152,6 +152,7 @@ void transport_init_se_cmd(struct se_cmd *, const struct target_core_fabric_ops *, struct se_session *, u32, int, int, unsigned char *); sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64); +sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, unsigned char *, unsigned char *, u64, u32, int, int, int, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 1/4] target: factor out a new helper, target_cmd_init_cdb() @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma target_setup_cmd_from_cdb() is called after a successful call to transport_lookup_cmd_lun(). The new helper factors out the code that can be called before the call to transport_lookup_cmd_lun(). This helper will be used in an upcoming commit to address NULL pointer dereference. Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/target_core_transport.c | 16 ++++++++++++---- include/target/target_core_fabric.h | 1 + 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index e6b448f43071..f2f7c5b818cc 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1410,11 +1410,8 @@ void transport_init_se_cmd( } sense_reason_t -target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb) { - struct se_device *dev = cmd->se_dev; - sense_reason_t ret; - /* * Ensure that the received CDB is less than the max (252 + 8) bytes * for VARIABLE_LENGTH_CMD @@ -1448,6 +1445,17 @@ void transport_init_se_cmd( memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb)); trace_target_sequencer_start(cmd); + return 0; +} +EXPORT_SYMBOL(target_cmd_init_cdb); + +sense_reason_t +target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +{ + struct se_device *dev = cmd->se_dev; + sense_reason_t ret; + + target_cmd_init_cdb(cmd, cdb); ret = dev->transport->parse_cdb(cmd); if (ret == TCM_UNSUPPORTED_SCSI_OPCODE) diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 063f133e47c2..6a2bfcca0c98 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -152,6 +152,7 @@ void transport_init_se_cmd(struct se_cmd *, const struct target_core_fabric_ops *, struct se_session *, u32, int, int, unsigned char *); sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64); +sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, unsigned char *, unsigned char *, u64, u32, int, int, int, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-07 19:58 ` Sudhakar Panneerselvam -1 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma Initialization of orig_fe_lun is moved to transport_init_se_cmd() from transport_lookup_cmd_lun(). This helps for the cases where the scsi request fails before the call to transport_lookup_cmd_lun() so that trace_target_cmd_complete() can print the LUN information to the trace buffer. Due to this change, the lun parameter is removed from transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 11 +++++------ drivers/target/target_core_device.c | 19 ++++++++----------- drivers/target/target_core_tmr.c | 4 ++-- drivers/target/target_core_transport.c | 12 +++++++----- drivers/target/target_core_xcopy.c | 4 ++-- drivers/usb/gadget/function/f_tcm.c | 6 ++++-- include/target/target_core_fabric.h | 6 +++--- 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 59379d662626..1b453629b516 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1158,7 +1158,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops, conn->sess->se_sess, be32_to_cpu(hdr->data_length), cmd->data_direction, sam_task_attr, - cmd->sense_buffer + 2); + cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun)); pr_debug("Got SCSI Command, ITT: 0x%08x, CmdSN: 0x%08x," " ExpXferLen: %u, Length: %u, CID: %hu\n", hdr->itt, @@ -1167,8 +1167,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, target_get_sess_cmd(&cmd->se_cmd, true); - cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd, - scsilun_to_int(&hdr->lun)); + cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; @@ -2000,7 +1999,8 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf) transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops, conn->sess->se_sess, 0, DMA_NONE, - TCM_SIMPLE_TAG, cmd->sense_buffer + 2); + TCM_SIMPLE_TAG, cmd->sense_buffer + 2, + scsilun_to_int(&hdr->lun)); target_get_sess_cmd(&cmd->se_cmd, true); @@ -2038,8 +2038,7 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf) * Locate the struct se_lun for all TMRs not related to ERL=2 TASK_REASSIGN */ if (function != ISCSI_TM_FUNC_TASK_REASSIGN) { - ret = transport_lookup_tmr_lun(&cmd->se_cmd, - scsilun_to_int(&hdr->lun)); + ret = transport_lookup_tmr_lun(&cmd->se_cmd); if (ret < 0) { se_tmr->response = ISCSI_TMF_RSP_NO_LUN; goto attach; diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 46b0e1ceb77f..405d82d44717 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -45,7 +45,7 @@ struct se_device *g_lun0_dev; sense_reason_t -transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) +transport_lookup_cmd_lun(struct se_cmd *se_cmd) { struct se_lun *se_lun = NULL; struct se_session *se_sess = se_cmd->se_sess; @@ -54,7 +54,7 @@ sense_reason_t ret = TCM_NO_SENSE; rcu_read_lock(); - deve = target_nacl_find_deve(nacl, unpacked_lun); + deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun); if (deve) { atomic_long_inc(&deve->total_cmds); @@ -74,7 +74,6 @@ se_cmd->se_lun = se_lun; se_cmd->pr_res_key = deve->pr_res_key; - se_cmd->orig_fe_lun = unpacked_lun; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; se_cmd->lun_ref_active = true; @@ -83,7 +82,7 @@ pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" " Access for 0x%08llx\n", se_cmd->se_tfo->fabric_name, - unpacked_lun); + se_cmd->orig_fe_lun); rcu_read_unlock(); ret = TCM_WRITE_PROTECTED; goto ref_dev; @@ -98,18 +97,17 @@ * REPORT_LUNS, et al to be returned when no active * MappedLUN=0 exists for this Initiator Port. */ - if (unpacked_lun != 0) { + if (se_cmd->orig_fe_lun != 0) { pr_err("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN" " Access for 0x%08llx from %s\n", se_cmd->se_tfo->fabric_name, - unpacked_lun, + se_cmd->orig_fe_lun, nacl->initiatorname); return TCM_NON_EXISTENT_LUN; } se_lun = se_sess->se_tpg->tpg_virt_lun0; se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0; - se_cmd->orig_fe_lun = 0; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; percpu_ref_get(&se_lun->lun_ref); @@ -145,7 +143,7 @@ } EXPORT_SYMBOL(transport_lookup_cmd_lun); -int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) +int transport_lookup_tmr_lun(struct se_cmd *se_cmd) { struct se_dev_entry *deve; struct se_lun *se_lun = NULL; @@ -155,7 +153,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) unsigned long flags; rcu_read_lock(); - deve = target_nacl_find_deve(nacl, unpacked_lun); + deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun); if (deve) { se_lun = rcu_dereference(deve->se_lun); @@ -166,7 +164,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) se_cmd->se_lun = se_lun; se_cmd->pr_res_key = deve->pr_res_key; - se_cmd->orig_fe_lun = unpacked_lun; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; se_cmd->lun_ref_active = true; } @@ -177,7 +174,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) pr_debug("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN" " Access for 0x%08llx for %s\n", se_cmd->se_tfo->fabric_name, - unpacked_lun, + se_cmd->orig_fe_lun, nacl->initiatorname); return -ENODEV; } diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index afbd492c76a9..89c84d472cd7 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -148,8 +148,8 @@ void core_tmr_abort_task( * code. */ if (!tmr->tmr_dev) - WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd, - se_cmd->orig_fe_lun) < 0); + WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd) < + 0); target_put_cmd_and_wait(se_cmd); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index f2f7c5b818cc..7ea77933e64d 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1364,7 +1364,7 @@ void transport_init_se_cmd( u32 data_length, int data_direction, int task_attr, - unsigned char *sense_buffer) + unsigned char *sense_buffer, u64 unpacked_lun) { INIT_LIST_HEAD(&cmd->se_delayed_node); INIT_LIST_HEAD(&cmd->se_qf_node); @@ -1383,6 +1383,7 @@ void transport_init_se_cmd( cmd->data_direction = data_direction; cmd->sam_task_attr = task_attr; cmd->sense_buffer = sense_buffer; + cmd->orig_fe_lun = unpacked_lun; cmd->state_active = false; } @@ -1596,7 +1597,8 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess * target_core_fabric_ops->queue_status() callback */ transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, - data_length, data_dir, task_attr, sense); + data_length, data_dir, task_attr, sense, + unpacked_lun); if (flags & TARGET_SCF_USE_CPUID) se_cmd->se_cmd_flags |= SCF_USE_CPUID; @@ -1622,7 +1624,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess /* * Locate se_lun pointer and attach it to struct se_cmd */ - rc = transport_lookup_cmd_lun(se_cmd, unpacked_lun); + rc = transport_lookup_cmd_lun(se_cmd); if (rc) { transport_send_check_condition_and_sense(se_cmd, rc, 0); target_put_sess_cmd(se_cmd); @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, BUG_ON(!se_tpg); transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun); /* * FIXME: Currently expect caller to handle se_cmd->se_tmr_req * allocation failure. @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, goto failure; } - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); + ret = transport_lookup_tmr_lun(se_cmd); if (ret) goto failure; diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index bd3ed6ce7571..5f1bab37a6bf 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -585,7 +585,7 @@ static int target_xcopy_read_source( (unsigned long long)src_lba, src_sectors, length); transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, - DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0]); + DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0], remote_port); @@ -630,7 +630,7 @@ static int target_xcopy_write_destination( (unsigned long long)dst_lba, dst_sectors, length); transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, - DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0]); + DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0], remote_port); diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c index 36504931b2d1..ac049909a8bf 100644 --- a/drivers/usb/gadget/function/f_tcm.c +++ b/drivers/usb/gadget/function/f_tcm.c @@ -1049,7 +1049,8 @@ static void usbg_cmd_work(struct work_struct *work) transport_init_se_cmd(se_cmd, tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo, tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE, - cmd->prio_attr, cmd->sense_iu.sense); + cmd->prio_attr, cmd->sense_iu.sense, + cmd->unpacked_lun); goto out; } @@ -1179,7 +1180,8 @@ static void bot_cmd_work(struct work_struct *work) transport_init_se_cmd(se_cmd, tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo, tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE, - cmd->prio_attr, cmd->sense_iu.sense); + cmd->prio_attr, cmd->sense_iu.sense, + cmd->unpacked_lun); goto out; } diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 6a2bfcca0c98..92f6cb20775e 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -150,8 +150,8 @@ void transport_register_session(struct se_portal_group *, void transport_init_se_cmd(struct se_cmd *, const struct target_core_fabric_ops *, - struct se_session *, u32, int, int, unsigned char *); -sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64); + struct se_session *, u32, int, int, unsigned char *, u64); +sense_reason_t transport_lookup_cmd_lun(struct se_cmd *); sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, @@ -188,7 +188,7 @@ int transport_send_check_condition_and_sense(struct se_cmd *, void core_tmr_release_req(struct se_tmr_req *); int transport_generic_handle_tmr(struct se_cmd *); void transport_generic_request_failure(struct se_cmd *, sense_reason_t); -int transport_lookup_tmr_lun(struct se_cmd *, u64); +int transport_lookup_tmr_lun(struct se_cmd *); void core_allocate_nexus_loss_ua(struct se_node_acl *acl); struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma Initialization of orig_fe_lun is moved to transport_init_se_cmd() from transport_lookup_cmd_lun(). This helps for the cases where the scsi request fails before the call to transport_lookup_cmd_lun() so that trace_target_cmd_complete() can print the LUN information to the trace buffer. Due to this change, the lun parameter is removed from transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 11 +++++------ drivers/target/target_core_device.c | 19 ++++++++----------- drivers/target/target_core_tmr.c | 4 ++-- drivers/target/target_core_transport.c | 12 +++++++----- drivers/target/target_core_xcopy.c | 4 ++-- drivers/usb/gadget/function/f_tcm.c | 6 ++++-- include/target/target_core_fabric.h | 6 +++--- 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 59379d662626..1b453629b516 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1158,7 +1158,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops, conn->sess->se_sess, be32_to_cpu(hdr->data_length), cmd->data_direction, sam_task_attr, - cmd->sense_buffer + 2); + cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun)); pr_debug("Got SCSI Command, ITT: 0x%08x, CmdSN: 0x%08x," " ExpXferLen: %u, Length: %u, CID: %hu\n", hdr->itt, @@ -1167,8 +1167,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, target_get_sess_cmd(&cmd->se_cmd, true); - cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd, - scsilun_to_int(&hdr->lun)); + cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; @@ -2000,7 +1999,8 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf) transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops, conn->sess->se_sess, 0, DMA_NONE, - TCM_SIMPLE_TAG, cmd->sense_buffer + 2); + TCM_SIMPLE_TAG, cmd->sense_buffer + 2, + scsilun_to_int(&hdr->lun)); target_get_sess_cmd(&cmd->se_cmd, true); @@ -2038,8 +2038,7 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf) * Locate the struct se_lun for all TMRs not related to ERL=2 TASK_REASSIGN */ if (function != ISCSI_TM_FUNC_TASK_REASSIGN) { - ret = transport_lookup_tmr_lun(&cmd->se_cmd, - scsilun_to_int(&hdr->lun)); + ret = transport_lookup_tmr_lun(&cmd->se_cmd); if (ret < 0) { se_tmr->response = ISCSI_TMF_RSP_NO_LUN; goto attach; diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 46b0e1ceb77f..405d82d44717 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -45,7 +45,7 @@ struct se_device *g_lun0_dev; sense_reason_t -transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun) +transport_lookup_cmd_lun(struct se_cmd *se_cmd) { struct se_lun *se_lun = NULL; struct se_session *se_sess = se_cmd->se_sess; @@ -54,7 +54,7 @@ sense_reason_t ret = TCM_NO_SENSE; rcu_read_lock(); - deve = target_nacl_find_deve(nacl, unpacked_lun); + deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun); if (deve) { atomic_long_inc(&deve->total_cmds); @@ -74,7 +74,6 @@ se_cmd->se_lun = se_lun; se_cmd->pr_res_key = deve->pr_res_key; - se_cmd->orig_fe_lun = unpacked_lun; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; se_cmd->lun_ref_active = true; @@ -83,7 +82,7 @@ pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN" " Access for 0x%08llx\n", se_cmd->se_tfo->fabric_name, - unpacked_lun); + se_cmd->orig_fe_lun); rcu_read_unlock(); ret = TCM_WRITE_PROTECTED; goto ref_dev; @@ -98,18 +97,17 @@ * REPORT_LUNS, et al to be returned when no active * MappedLUN=0 exists for this Initiator Port. */ - if (unpacked_lun != 0) { + if (se_cmd->orig_fe_lun != 0) { pr_err("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN" " Access for 0x%08llx from %s\n", se_cmd->se_tfo->fabric_name, - unpacked_lun, + se_cmd->orig_fe_lun, nacl->initiatorname); return TCM_NON_EXISTENT_LUN; } se_lun = se_sess->se_tpg->tpg_virt_lun0; se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0; - se_cmd->orig_fe_lun = 0; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; percpu_ref_get(&se_lun->lun_ref); @@ -145,7 +143,7 @@ } EXPORT_SYMBOL(transport_lookup_cmd_lun); -int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) +int transport_lookup_tmr_lun(struct se_cmd *se_cmd) { struct se_dev_entry *deve; struct se_lun *se_lun = NULL; @@ -155,7 +153,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) unsigned long flags; rcu_read_lock(); - deve = target_nacl_find_deve(nacl, unpacked_lun); + deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun); if (deve) { se_lun = rcu_dereference(deve->se_lun); @@ -166,7 +164,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) se_cmd->se_lun = se_lun; se_cmd->pr_res_key = deve->pr_res_key; - se_cmd->orig_fe_lun = unpacked_lun; se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD; se_cmd->lun_ref_active = true; } @@ -177,7 +174,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun) pr_debug("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN" " Access for 0x%08llx for %s\n", se_cmd->se_tfo->fabric_name, - unpacked_lun, + se_cmd->orig_fe_lun, nacl->initiatorname); return -ENODEV; } diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c index afbd492c76a9..89c84d472cd7 100644 --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -148,8 +148,8 @@ void core_tmr_abort_task( * code. */ if (!tmr->tmr_dev) - WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd, - se_cmd->orig_fe_lun) < 0); + WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd) < + 0); target_put_cmd_and_wait(se_cmd); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index f2f7c5b818cc..7ea77933e64d 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1364,7 +1364,7 @@ void transport_init_se_cmd( u32 data_length, int data_direction, int task_attr, - unsigned char *sense_buffer) + unsigned char *sense_buffer, u64 unpacked_lun) { INIT_LIST_HEAD(&cmd->se_delayed_node); INIT_LIST_HEAD(&cmd->se_qf_node); @@ -1383,6 +1383,7 @@ void transport_init_se_cmd( cmd->data_direction = data_direction; cmd->sam_task_attr = task_attr; cmd->sense_buffer = sense_buffer; + cmd->orig_fe_lun = unpacked_lun; cmd->state_active = false; } @@ -1596,7 +1597,8 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess * target_core_fabric_ops->queue_status() callback */ transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, - data_length, data_dir, task_attr, sense); + data_length, data_dir, task_attr, sense, + unpacked_lun); if (flags & TARGET_SCF_USE_CPUID) se_cmd->se_cmd_flags |= SCF_USE_CPUID; @@ -1622,7 +1624,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess /* * Locate se_lun pointer and attach it to struct se_cmd */ - rc = transport_lookup_cmd_lun(se_cmd, unpacked_lun); + rc = transport_lookup_cmd_lun(se_cmd); if (rc) { transport_send_check_condition_and_sense(se_cmd, rc, 0); target_put_sess_cmd(se_cmd); @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, BUG_ON(!se_tpg); transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun); /* * FIXME: Currently expect caller to handle se_cmd->se_tmr_req * allocation failure. @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess, goto failure; } - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); + ret = transport_lookup_tmr_lun(se_cmd); if (ret) goto failure; diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index bd3ed6ce7571..5f1bab37a6bf 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -585,7 +585,7 @@ static int target_xcopy_read_source( (unsigned long long)src_lba, src_sectors, length); transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, - DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0]); + DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0], remote_port); @@ -630,7 +630,7 @@ static int target_xcopy_write_destination( (unsigned long long)dst_lba, dst_sectors, length); transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length, - DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0]); + DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0); rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0], remote_port); diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c index 36504931b2d1..ac049909a8bf 100644 --- a/drivers/usb/gadget/function/f_tcm.c +++ b/drivers/usb/gadget/function/f_tcm.c @@ -1049,7 +1049,8 @@ static void usbg_cmd_work(struct work_struct *work) transport_init_se_cmd(se_cmd, tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo, tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE, - cmd->prio_attr, cmd->sense_iu.sense); + cmd->prio_attr, cmd->sense_iu.sense, + cmd->unpacked_lun); goto out; } @@ -1179,7 +1180,8 @@ static void bot_cmd_work(struct work_struct *work) transport_init_se_cmd(se_cmd, tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo, tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE, - cmd->prio_attr, cmd->sense_iu.sense); + cmd->prio_attr, cmd->sense_iu.sense, + cmd->unpacked_lun); goto out; } diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 6a2bfcca0c98..92f6cb20775e 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -150,8 +150,8 @@ void transport_register_session(struct se_portal_group *, void transport_init_se_cmd(struct se_cmd *, const struct target_core_fabric_ops *, - struct se_session *, u32, int, int, unsigned char *); -sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64); + struct se_session *, u32, int, int, unsigned char *, u64); +sense_reason_t transport_lookup_cmd_lun(struct se_cmd *); sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, @@ -188,7 +188,7 @@ int transport_send_check_condition_and_sense(struct se_cmd *, void core_tmr_release_req(struct se_tmr_req *); int transport_generic_handle_tmr(struct se_cmd *); void transport_generic_request_failure(struct se_cmd *, sense_reason_t); -int transport_lookup_tmr_lun(struct se_cmd *, u64); +int transport_lookup_tmr_lun(struct se_cmd *); void core_allocate_nexus_loss_ua(struct se_node_acl *acl); struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-09-02 13:49 ` Martin Wilck -1 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-02 13:49 UTC (permalink / raw) To: Sudhakar Panneerselvam, martin.petersen, michael.christie, target-devel, linux-scsi Cc: shirley.ma, Hannes Reinecke, Daniel Wagner, Arun Easi Hello Sudhakar, On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote: > Initialization of orig_fe_lun is moved to transport_init_se_cmd() > from > transport_lookup_cmd_lun(). This helps for the cases where the scsi > request > fails before the call to transport_lookup_cmd_lun() so that > trace_target_cmd_complete() can print the LUN information to the > trace > buffer. Due to this change, the lun parameter is removed from > transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). > > Signed-off-by: Sudhakar Panneerselvam < > sudhakar.panneerselvam@oracle.com> > --- > drivers/target/iscsi/iscsi_target.c | 11 +++++------ > drivers/target/target_core_device.c | 19 ++++++++----------- > drivers/target/target_core_tmr.c | 4 ++-- > drivers/target/target_core_transport.c | 12 +++++++----- > drivers/target/target_core_xcopy.c | 4 ++-- > drivers/usb/gadget/function/f_tcm.c | 6 ++++-- > include/target/target_core_fabric.h | 6 +++--- > 7 files changed, 31 insertions(+), 31 deletions(-) > > [...] > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c > index f2f7c5b818cc..7ea77933e64d 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > [...] > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > struct se_session *se_sess, > BUG_ON(!se_tpg); > > transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, > - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); > + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, > unpacked_lun); > /* > * FIXME: Currently expect caller to handle se_cmd->se_tmr_req > * allocation failure. Between this hunk and the next one in target_submit_tmr(), there is this code: /* * If this is ABORT_TASK with no explicit fabric provided LUN, * go ahead and search active session tags for a match to figure * out unpacked_lun for the original se_cmd. */ if (tm_type = TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) { if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun)) goto failure; } > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > struct se_session *se_sess, > goto failure; > } > > - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); > + ret = transport_lookup_tmr_lun(se_cmd); > if (ret) > goto failure; > AFAICS, your patch breaks the case where the above code is executed to derive unpacked_lun from the tag. The updated value of unpacked_lun is never used. That would break aborts for the qla2xxx target. Am I overlooking something? Can you please explain? Regards Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). @ 2020-09-02 13:49 ` Martin Wilck 0 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-02 13:49 UTC (permalink / raw) To: Sudhakar Panneerselvam, martin.petersen, michael.christie, target-devel, linux-scsi Cc: shirley.ma, Hannes Reinecke, Daniel Wagner, Arun Easi Hello Sudhakar, On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote: > Initialization of orig_fe_lun is moved to transport_init_se_cmd() > from > transport_lookup_cmd_lun(). This helps for the cases where the scsi > request > fails before the call to transport_lookup_cmd_lun() so that > trace_target_cmd_complete() can print the LUN information to the > trace > buffer. Due to this change, the lun parameter is removed from > transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). > > Signed-off-by: Sudhakar Panneerselvam < > sudhakar.panneerselvam@oracle.com> > --- > drivers/target/iscsi/iscsi_target.c | 11 +++++------ > drivers/target/target_core_device.c | 19 ++++++++----------- > drivers/target/target_core_tmr.c | 4 ++-- > drivers/target/target_core_transport.c | 12 +++++++----- > drivers/target/target_core_xcopy.c | 4 ++-- > drivers/usb/gadget/function/f_tcm.c | 6 ++++-- > include/target/target_core_fabric.h | 6 +++--- > 7 files changed, 31 insertions(+), 31 deletions(-) > > [...] > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c > index f2f7c5b818cc..7ea77933e64d 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > [...] > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > struct se_session *se_sess, > BUG_ON(!se_tpg); > > transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, > - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); > + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, > unpacked_lun); > /* > * FIXME: Currently expect caller to handle se_cmd->se_tmr_req > * allocation failure. Between this hunk and the next one in target_submit_tmr(), there is this code: /* * If this is ABORT_TASK with no explicit fabric provided LUN, * go ahead and search active session tags for a match to figure * out unpacked_lun for the original se_cmd. */ if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) { if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun)) goto failure; } > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > struct se_session *se_sess, > goto failure; > } > > - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); > + ret = transport_lookup_tmr_lun(se_cmd); > if (ret) > goto failure; > AFAICS, your patch breaks the case where the above code is executed to derive unpacked_lun from the tag. The updated value of unpacked_lun is never used. That would break aborts for the qla2xxx target. Am I overlooking something? Can you please explain? Regards Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). 2020-09-02 13:49 ` Martin Wilck @ 2020-09-02 15:14 ` Sudhakar Panneerselvam -1 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-09-02 15:14 UTC (permalink / raw) To: Martin Wilck, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner, Arun Easi Hi Martin, > -----Original Message----- > From: Martin Wilck [mailto:mwilck@suse.com] > Sent: Wednesday, September 2, 2020 7:50 AM > To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>; Martin > Petersen <martin.petersen@oracle.com>; Michael Christie > <michael.christie@oracle.com>; target-devel@vger.kernel.org; linux- > scsi@vger.kernel.org > Cc: Shirley Ma <shirley.ma@oracle.com>; Hannes Reinecke <hare@suse.com>; > Daniel Wagner <daniel.wagner@suse.com>; Arun Easi <aeasi@marvell.com> > Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). > > Hello Sudhakar, > > On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote: > > Initialization of orig_fe_lun is moved to transport_init_se_cmd() > > from > > transport_lookup_cmd_lun(). This helps for the cases where the scsi > > request > > fails before the call to transport_lookup_cmd_lun() so that > > trace_target_cmd_complete() can print the LUN information to the > > trace > > buffer. Due to this change, the lun parameter is removed from > > transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). > > > > Signed-off-by: Sudhakar Panneerselvam < > > sudhakar.panneerselvam@oracle.com> > > --- > > drivers/target/iscsi/iscsi_target.c | 11 +++++------ > > drivers/target/target_core_device.c | 19 ++++++++----------- > > drivers/target/target_core_tmr.c | 4 ++-- > > drivers/target/target_core_transport.c | 12 +++++++----- > > drivers/target/target_core_xcopy.c | 4 ++-- > > drivers/usb/gadget/function/f_tcm.c | 6 ++++-- > > include/target/target_core_fabric.h | 6 +++--- > > 7 files changed, 31 insertions(+), 31 deletions(-) > > > > [...] > > diff --git a/drivers/target/target_core_transport.c > > b/drivers/target/target_core_transport.c > > index f2f7c5b818cc..7ea77933e64d 100644 > > --- a/drivers/target/target_core_transport.c > > +++ b/drivers/target/target_core_transport.c > > [...] > > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > > struct se_session *se_sess, > > BUG_ON(!se_tpg); > > > > transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, > > - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); > > + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, > > unpacked_lun); > > /* > > * FIXME: Currently expect caller to handle se_cmd->se_tmr_req > > * allocation failure. > > Between this hunk and the next one in target_submit_tmr(), there > is this code: > > /* > * If this is ABORT_TASK with no explicit fabric provided LUN, > * go ahead and search active session tags for a match to figure > * out unpacked_lun for the original se_cmd. > */ > if (tm_type == TMR_ABORT_TASK && (flags & > TARGET_SCF_LOOKUP_LUN_FROM_TAG)) { > if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun)) > goto failure; > } > > > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > > struct se_session *se_sess, > > goto failure; > > } > > > > - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); > > + ret = transport_lookup_tmr_lun(se_cmd); > > if (ret) > > goto failure; > > > > AFAICS, your patch breaks the case where the above code is executed to > derive unpacked_lun from the tag. The updated value of unpacked_lun is > never used. That would break aborts for the qla2xxx target. > > Am I overlooking something? Can you please explain? > You are right. This change breaks the qlogic abort task code path, since it is the only transport that sets the TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My apologies. I can send out a patch if you have not written one already. Please let me know. Thanks Sudhakar > Regards > Martin > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). @ 2020-09-02 15:14 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-09-02 15:14 UTC (permalink / raw) To: Martin Wilck, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner, Arun Easi Hi Martin, > -----Original Message----- > From: Martin Wilck [mailto:mwilck@suse.com] > Sent: Wednesday, September 2, 2020 7:50 AM > To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>; Martin > Petersen <martin.petersen@oracle.com>; Michael Christie > <michael.christie@oracle.com>; target-devel@vger.kernel.org; linux- > scsi@vger.kernel.org > Cc: Shirley Ma <shirley.ma@oracle.com>; Hannes Reinecke <hare@suse.com>; > Daniel Wagner <daniel.wagner@suse.com>; Arun Easi <aeasi@marvell.com> > Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). > > Hello Sudhakar, > > On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote: > > Initialization of orig_fe_lun is moved to transport_init_se_cmd() > > from > > transport_lookup_cmd_lun(). This helps for the cases where the scsi > > request > > fails before the call to transport_lookup_cmd_lun() so that > > trace_target_cmd_complete() can print the LUN information to the > > trace > > buffer. Due to this change, the lun parameter is removed from > > transport_lookup_cmd_lun() and transport_lookup_tmr_lun(). > > > > Signed-off-by: Sudhakar Panneerselvam < > > sudhakar.panneerselvam@oracle.com> > > --- > > drivers/target/iscsi/iscsi_target.c | 11 +++++------ > > drivers/target/target_core_device.c | 19 ++++++++----------- > > drivers/target/target_core_tmr.c | 4 ++-- > > drivers/target/target_core_transport.c | 12 +++++++----- > > drivers/target/target_core_xcopy.c | 4 ++-- > > drivers/usb/gadget/function/f_tcm.c | 6 ++++-- > > include/target/target_core_fabric.h | 6 +++--- > > 7 files changed, 31 insertions(+), 31 deletions(-) > > > > [...] > > diff --git a/drivers/target/target_core_transport.c > > b/drivers/target/target_core_transport.c > > index f2f7c5b818cc..7ea77933e64d 100644 > > --- a/drivers/target/target_core_transport.c > > +++ b/drivers/target/target_core_transport.c > > [...] > > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > > struct se_session *se_sess, > > BUG_ON(!se_tpg); > > > > transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, > > - 0, DMA_NONE, TCM_SIMPLE_TAG, sense); > > + 0, DMA_NONE, TCM_SIMPLE_TAG, sense, > > unpacked_lun); > > /* > > * FIXME: Currently expect caller to handle se_cmd->se_tmr_req > > * allocation failure. > > Between this hunk and the next one in target_submit_tmr(), there > is this code: > > /* > * If this is ABORT_TASK with no explicit fabric provided LUN, > * go ahead and search active session tags for a match to figure > * out unpacked_lun for the original se_cmd. > */ > if (tm_type == TMR_ABORT_TASK && (flags & > TARGET_SCF_LOOKUP_LUN_FROM_TAG)) { > if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun)) > goto failure; > } > > > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, > > struct se_session *se_sess, > > goto failure; > > } > > > > - ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun); > > + ret = transport_lookup_tmr_lun(se_cmd); > > if (ret) > > goto failure; > > > > AFAICS, your patch breaks the case where the above code is executed to > derive unpacked_lun from the tag. The updated value of unpacked_lun is > never used. That would break aborts for the qla2xxx target. > > Am I overlooking something? Can you please explain? > You are right. This change breaks the qlogic abort task code path, since it is the only transport that sets the TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My apologies. I can send out a patch if you have not written one already. Please let me know. Thanks Sudhakar > Regards > Martin > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). 2020-09-02 15:14 ` Sudhakar Panneerselvam @ 2020-09-03 13:00 ` Martin Wilck -1 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-03 13:00 UTC (permalink / raw) To: Sudhakar Panneerselvam, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner, Arun Easi On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote: > Hi Martin, > > > > > AFAICS, your patch breaks the case where the above code is executed > > to > > derive unpacked_lun from the tag. The updated value of unpacked_lun > > is > > never used. That would break aborts for the qla2xxx target. > > > > Am I overlooking something? Can you please explain? > > > > You are right. This change breaks the qlogic abort task code path, > since it is the only transport that sets the > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My > apologies. I can send out a patch if you have not written one > already. Please let me know. Please go ahead. I haven't written a patch - I'm not experienced enough with the target code to quickly grok whether simply moving the target_lookup_lun_from_tag() code upward would work, in particular wrt handling failures and cleaning up. Regards, Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). @ 2020-09-03 13:00 ` Martin Wilck 0 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-03 13:00 UTC (permalink / raw) To: Sudhakar Panneerselvam, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner, Arun Easi On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote: > Hi Martin, > > > > > AFAICS, your patch breaks the case where the above code is executed > > to > > derive unpacked_lun from the tag. The updated value of unpacked_lun > > is > > never used. That would break aborts for the qla2xxx target. > > > > Am I overlooking something? Can you please explain? > > > > You are right. This change breaks the qlogic abort task code path, > since it is the only transport that sets the > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My > apologies. I can send out a patch if you have not written one > already. Please let me know. Please go ahead. I haven't written a patch - I'm not experienced enough with the target code to quickly grok whether simply moving the target_lookup_lun_from_tag() code upward would work, in particular wrt handling failures and cleaning up. Regards, Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). 2020-09-03 13:00 ` Martin Wilck @ 2020-09-03 13:13 ` Martin Wilck -1 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-03 13:13 UTC (permalink / raw) To: Arun Easi, Sudhakar Panneerselvam, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote: > On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote: > > Hi Martin, > > > > > AFAICS, your patch breaks the case where the above code is > > > executed > > > to > > > derive unpacked_lun from the tag. The updated value of > > > unpacked_lun > > > is > > > never used. That would break aborts for the qla2xxx target. > > > > > > Am I overlooking something? Can you please explain? > > > > > > > You are right. This change breaks the qlogic abort task code path, > > since it is the only transport that sets the > > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My > > apologies. I can send out a patch if you have not written one > > already. Please let me know. > > Please go ahead. I haven't written a patch - I'm not experienced > enough > with the target code to quickly grok whether simply moving the > target_lookup_lun_from_tag() code upward would work, in particular > wrt > handling failures and cleaning up. We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be removed entirely. As you said, qla2xxx is the only user of this feature. And it actually uses it only in a single code path, __qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand, AFAICT). If you look at the code of __qlt_24xx_handle_abts(), you can see that it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list until a matching tag is found. It would clearly be equivalent, and more efficient, if qla2xxx set the lun directly rather than relying on common code iterating through the same list again, later. Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd(). @ 2020-09-03 13:13 ` Martin Wilck 0 siblings, 0 replies; 22+ messages in thread From: Martin Wilck @ 2020-09-03 13:13 UTC (permalink / raw) To: Arun Easi, Sudhakar Panneerselvam, Martin Petersen, Michael Christie, target-devel, linux-scsi Cc: Shirley Ma, Hannes Reinecke, Daniel Wagner On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote: > On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote: > > Hi Martin, > > > > > AFAICS, your patch breaks the case where the above code is > > > executed > > > to > > > derive unpacked_lun from the tag. The updated value of > > > unpacked_lun > > > is > > > never used. That would break aborts for the qla2xxx target. > > > > > > Am I overlooking something? Can you please explain? > > > > > > > You are right. This change breaks the qlogic abort task code path, > > since it is the only transport that sets the > > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My > > apologies. I can send out a patch if you have not written one > > already. Please let me know. > > Please go ahead. I haven't written a patch - I'm not experienced > enough > with the target code to quickly grok whether simply moving the > target_lookup_lun_from_tag() code upward would work, in particular > wrt > handling failures and cleaning up. We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be removed entirely. As you said, qla2xxx is the only user of this feature. And it actually uses it only in a single code path, __qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand, AFAICT). If you look at the code of __qlt_24xx_handle_abts(), you can see that it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list until a matching tag is found. It would clearly be equivalent, and more efficient, if qla2xxx set the lun directly rather than relying on common code iterating through the same list again, later. Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 3/4] target: fix NULL pointer dereference 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-07 19:58 ` Sudhakar Panneerselvam -1 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma NULL pointer dereference happens when the following conditions are met 1) A SCSI command is received for a non-existing LU or cdb initialization fails in target_setup_cmd_from_cdb(). 2) Tracing is enabled. The following call sequences lead to NULL pointer dereference: 1) iscsit_setup_scsi_cmd transport_lookup_cmd_lun <-- lookup fails. or target_setup_cmd_from_cdb() <-- cdb initialization fails iscsit_process_scsi_cmd iscsit_sequence_cmd transport_send_check_condition_and_sense trace_target_cmd_complete <-- NULL dereference 2) target_submit_cmd_map_sgls transport_lookup_cmd_lun <-- lookup fails or target_setup_cmd_from_cdb() <-- cdb initialization fails transport_send_check_condition_and_sense trace_target_cmd_complete <-- NULL dereference In the above sequence, cmd->t_task_cdb is uninitialized which when referenced in trace_target_cmd_complete() causes NULL pointer dereference. The fix is to use the helper, target_cmd_init_cdb() and call it after transport_init_se_cmd() is called, so that cmd->t_task_cdb can be initialized and hence can be referenced in trace_target_cmd_complete(). Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 18 +++++++++++------- drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++++------ drivers/target/target_core_xcopy.c | 3 +++ 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 1b453629b516..93534dcc66c9 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1167,6 +1167,16 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, target_get_sess_cmd(&cmd->se_cmd, true); + cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb); + if (cmd->sense_reason) { + if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) { + return iscsit_add_reject_cmd(cmd, + ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); + } + + goto attach_cmd; + } + cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; @@ -1174,14 +1184,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, /* only used for printks or comparing with ->ref_task_tag */ cmd->se_cmd.tag = (__force u32)cmd->init_task_tag; cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb); - if (cmd->sense_reason) { - if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) { - return iscsit_add_reject_cmd(cmd, - ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); - } - + if (cmd->sense_reason) goto attach_cmd; - } if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) { return iscsit_add_reject_cmd(cmd, diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7ea77933e64d..0fbb38254535 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1413,6 +1413,9 @@ void transport_init_se_cmd( sense_reason_t target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb) { + sense_reason_t ret; + + cmd->t_task_cdb = &cmd->__t_task_cdb[0]; /* * Ensure that the received CDB is less than the max (252 + 8) bytes * for VARIABLE_LENGTH_CMD @@ -1421,7 +1424,8 @@ void transport_init_se_cmd( pr_err("Received SCSI CDB with command_size: %d that" " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n", scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE); - return TCM_INVALID_CDB_FIELD; + ret = TCM_INVALID_CDB_FIELD; + goto err; } /* * If the received CDB is larger than TCM_MAX_COMMAND_SIZE, @@ -1436,10 +1440,10 @@ void transport_init_se_cmd( " %u > sizeof(cmd->__t_task_cdb): %lu ops\n", scsi_command_size(cdb), (unsigned long)sizeof(cmd->__t_task_cdb)); - return TCM_OUT_OF_RESOURCES; + ret = TCM_OUT_OF_RESOURCES; + goto err; } - } else - cmd->t_task_cdb = &cmd->__t_task_cdb[0]; + } /* * Copy the original CDB into cmd-> */ @@ -1447,6 +1451,15 @@ void transport_init_se_cmd( trace_target_sequencer_start(cmd); return 0; + +err: + /* + * Copy the CDB here to allow trace_target_cmd_complete() to + * print the cdb to the trace buffers. + */ + memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), + (unsigned int)TCM_MAX_COMMAND_SIZE)); + return ret; } EXPORT_SYMBOL(target_cmd_init_cdb); @@ -1456,8 +1469,6 @@ void transport_init_se_cmd( struct se_device *dev = cmd->se_dev; sense_reason_t ret; - target_cmd_init_cdb(cmd, cdb); - ret = dev->transport->parse_cdb(cmd); if (ret = TCM_UNSUPPORTED_SCSI_OPCODE) pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n", @@ -1621,6 +1632,14 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess */ if (flags & TARGET_SCF_BIDI_OP) se_cmd->se_cmd_flags |= SCF_BIDI; + + rc = target_cmd_init_cdb(se_cmd, cdb); + if (rc) { + transport_send_check_condition_and_sense(se_cmd, rc, 0); + target_put_sess_cmd(se_cmd); + return 0; + } + /* * Locate se_lun pointer and attach it to struct se_cmd */ diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 5f1bab37a6bf..7f9c93b5cba6 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd( } cmd->se_cmd_flags |= SCF_SE_LUN_CMD; + if (target_cmd_init_cdb(cmd, cdb)) + return -EINVAL; + cmd->tag = 0; if (target_setup_cmd_from_cdb(cmd, cdb)) return -EINVAL; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 3/4] target: fix NULL pointer dereference @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma NULL pointer dereference happens when the following conditions are met 1) A SCSI command is received for a non-existing LU or cdb initialization fails in target_setup_cmd_from_cdb(). 2) Tracing is enabled. The following call sequences lead to NULL pointer dereference: 1) iscsit_setup_scsi_cmd transport_lookup_cmd_lun <-- lookup fails. or target_setup_cmd_from_cdb() <-- cdb initialization fails iscsit_process_scsi_cmd iscsit_sequence_cmd transport_send_check_condition_and_sense trace_target_cmd_complete <-- NULL dereference 2) target_submit_cmd_map_sgls transport_lookup_cmd_lun <-- lookup fails or target_setup_cmd_from_cdb() <-- cdb initialization fails transport_send_check_condition_and_sense trace_target_cmd_complete <-- NULL dereference In the above sequence, cmd->t_task_cdb is uninitialized which when referenced in trace_target_cmd_complete() causes NULL pointer dereference. The fix is to use the helper, target_cmd_init_cdb() and call it after transport_init_se_cmd() is called, so that cmd->t_task_cdb can be initialized and hence can be referenced in trace_target_cmd_complete(). Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 18 +++++++++++------- drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++++------ drivers/target/target_core_xcopy.c | 3 +++ 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 1b453629b516..93534dcc66c9 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1167,6 +1167,16 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, target_get_sess_cmd(&cmd->se_cmd, true); + cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb); + if (cmd->sense_reason) { + if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) { + return iscsit_add_reject_cmd(cmd, + ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); + } + + goto attach_cmd; + } + cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; @@ -1174,14 +1184,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, /* only used for printks or comparing with ->ref_task_tag */ cmd->se_cmd.tag = (__force u32)cmd->init_task_tag; cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb); - if (cmd->sense_reason) { - if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) { - return iscsit_add_reject_cmd(cmd, - ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf); - } - + if (cmd->sense_reason) goto attach_cmd; - } if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) { return iscsit_add_reject_cmd(cmd, diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7ea77933e64d..0fbb38254535 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1413,6 +1413,9 @@ void transport_init_se_cmd( sense_reason_t target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb) { + sense_reason_t ret; + + cmd->t_task_cdb = &cmd->__t_task_cdb[0]; /* * Ensure that the received CDB is less than the max (252 + 8) bytes * for VARIABLE_LENGTH_CMD @@ -1421,7 +1424,8 @@ void transport_init_se_cmd( pr_err("Received SCSI CDB with command_size: %d that" " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n", scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE); - return TCM_INVALID_CDB_FIELD; + ret = TCM_INVALID_CDB_FIELD; + goto err; } /* * If the received CDB is larger than TCM_MAX_COMMAND_SIZE, @@ -1436,10 +1440,10 @@ void transport_init_se_cmd( " %u > sizeof(cmd->__t_task_cdb): %lu ops\n", scsi_command_size(cdb), (unsigned long)sizeof(cmd->__t_task_cdb)); - return TCM_OUT_OF_RESOURCES; + ret = TCM_OUT_OF_RESOURCES; + goto err; } - } else - cmd->t_task_cdb = &cmd->__t_task_cdb[0]; + } /* * Copy the original CDB into cmd-> */ @@ -1447,6 +1451,15 @@ void transport_init_se_cmd( trace_target_sequencer_start(cmd); return 0; + +err: + /* + * Copy the CDB here to allow trace_target_cmd_complete() to + * print the cdb to the trace buffers. + */ + memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), + (unsigned int)TCM_MAX_COMMAND_SIZE)); + return ret; } EXPORT_SYMBOL(target_cmd_init_cdb); @@ -1456,8 +1469,6 @@ void transport_init_se_cmd( struct se_device *dev = cmd->se_dev; sense_reason_t ret; - target_cmd_init_cdb(cmd, cdb); - ret = dev->transport->parse_cdb(cmd); if (ret == TCM_UNSUPPORTED_SCSI_OPCODE) pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n", @@ -1621,6 +1632,14 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess */ if (flags & TARGET_SCF_BIDI_OP) se_cmd->se_cmd_flags |= SCF_BIDI; + + rc = target_cmd_init_cdb(se_cmd, cdb); + if (rc) { + transport_send_check_condition_and_sense(se_cmd, rc, 0); + target_put_sess_cmd(se_cmd); + return 0; + } + /* * Locate se_lun pointer and attach it to struct se_cmd */ diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 5f1bab37a6bf..7f9c93b5cba6 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd( } cmd->se_cmd_flags |= SCF_SE_LUN_CMD; + if (target_cmd_init_cdb(cmd, cdb)) + return -EINVAL; + cmd->tag = 0; if (target_setup_cmd_from_cdb(cmd, cdb)) return -EINVAL; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 4/4] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-07 19:58 ` Sudhakar Panneerselvam -1 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma This commit also removes the unused argument, cdb that was passed to this function. Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 2 +- drivers/target/target_core_transport.c | 6 +++--- drivers/target/target_core_xcopy.c | 2 +- include/target/target_core_fabric.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 93534dcc66c9..c9689610e186 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1183,7 +1183,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, /* only used for printks or comparing with ->ref_task_tag */ cmd->se_cmd.tag = (__force u32)cmd->init_task_tag; - cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb); + cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 0fbb38254535..229407d81613 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1464,7 +1464,7 @@ void transport_init_se_cmd( EXPORT_SYMBOL(target_cmd_init_cdb); sense_reason_t -target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +target_cmd_parse_cdb(struct se_cmd *cmd) { struct se_device *dev = cmd->se_dev; sense_reason_t ret; @@ -1486,7 +1486,7 @@ void transport_init_se_cmd( atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus); return 0; } -EXPORT_SYMBOL(target_setup_cmd_from_cdb); +EXPORT_SYMBOL(target_cmd_parse_cdb); /* * Used by fabric module frontends to queue tasks directly. @@ -1650,7 +1650,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess return 0; } - rc = target_setup_cmd_from_cdb(se_cmd, cdb); + rc = target_cmd_parse_cdb(se_cmd); if (rc != 0) { transport_generic_request_failure(se_cmd, rc); return 0; diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 7f9c93b5cba6..0d00ccbeb050 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd( return -EINVAL; cmd->tag = 0; - if (target_setup_cmd_from_cdb(cmd, cdb)) + if (target_cmd_parse_cdb(cmd)) return -EINVAL; if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg, diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 92f6cb20775e..6adf4d71acf6 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -153,7 +153,7 @@ void transport_init_se_cmd(struct se_cmd *, struct se_session *, u32, int, int, unsigned char *, u64); sense_reason_t transport_lookup_cmd_lun(struct se_cmd *); sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); -sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); +sense_reason_t target_cmd_parse_cdb(struct se_cmd *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, unsigned char *, unsigned char *, u64, u32, int, int, int, struct scatterlist *, u32, struct scatterlist *, u32, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 4/4] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() @ 2020-06-07 19:58 ` Sudhakar Panneerselvam 0 siblings, 0 replies; 22+ messages in thread From: Sudhakar Panneerselvam @ 2020-06-07 19:58 UTC (permalink / raw) To: martin.petersen, michael.christie, target-devel, linux-scsi; +Cc: shirley.ma This commit also removes the unused argument, cdb that was passed to this function. Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com> --- drivers/target/iscsi/iscsi_target.c | 2 +- drivers/target/target_core_transport.c | 6 +++--- drivers/target/target_core_xcopy.c | 2 +- include/target/target_core_fabric.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 93534dcc66c9..c9689610e186 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -1183,7 +1183,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, /* only used for printks or comparing with ->ref_task_tag */ cmd->se_cmd.tag = (__force u32)cmd->init_task_tag; - cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb); + cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd); if (cmd->sense_reason) goto attach_cmd; diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 0fbb38254535..229407d81613 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1464,7 +1464,7 @@ void transport_init_se_cmd( EXPORT_SYMBOL(target_cmd_init_cdb); sense_reason_t -target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb) +target_cmd_parse_cdb(struct se_cmd *cmd) { struct se_device *dev = cmd->se_dev; sense_reason_t ret; @@ -1486,7 +1486,7 @@ void transport_init_se_cmd( atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus); return 0; } -EXPORT_SYMBOL(target_setup_cmd_from_cdb); +EXPORT_SYMBOL(target_cmd_parse_cdb); /* * Used by fabric module frontends to queue tasks directly. @@ -1650,7 +1650,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess return 0; } - rc = target_setup_cmd_from_cdb(se_cmd, cdb); + rc = target_cmd_parse_cdb(se_cmd); if (rc != 0) { transport_generic_request_failure(se_cmd, rc); return 0; diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c index 7f9c93b5cba6..0d00ccbeb050 100644 --- a/drivers/target/target_core_xcopy.c +++ b/drivers/target/target_core_xcopy.c @@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd( return -EINVAL; cmd->tag = 0; - if (target_setup_cmd_from_cdb(cmd, cdb)) + if (target_cmd_parse_cdb(cmd)) return -EINVAL; if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg, diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 92f6cb20775e..6adf4d71acf6 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -153,7 +153,7 @@ void transport_init_se_cmd(struct se_cmd *, struct se_session *, u32, int, int, unsigned char *, u64); sense_reason_t transport_lookup_cmd_lun(struct se_cmd *); sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *); -sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *); +sense_reason_t target_cmd_parse_cdb(struct se_cmd *); int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *, unsigned char *, unsigned char *, u64, u32, int, int, int, struct scatterlist *, u32, struct scatterlist *, u32, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 0/4] target: fix NULL pointer dereference 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-07 20:37 ` Mike Christie -1 siblings, 0 replies; 22+ messages in thread From: Mike Christie @ 2020-06-07 20:37 UTC (permalink / raw) To: Sudhakar Panneerselvam, martin.petersen, target-devel, linux-scsi Cc: shirley.ma On 6/7/20 2:58 PM, Sudhakar Panneerselvam wrote: > The following set of commits address a NULL pointer dereference and some > refactoring around this issue. > > v4: > - initialize the LUN in transport_init_se_cmd() > > v3: > - fix NULL pointer dereference when cdb initialization fails > > v2: > - new helper is named as target_cmd_init_cdb() > - existing function, target_setup_cmd_from_cdb is renamed as > target_cmd_parse_cdb() > > Sudhakar Panneerselvam (4): > target: factor out a new helper, target_cmd_init_cdb() > target: initialize LUN in transport_init_se_cmd(). > target: fix NULL pointer dereference > target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() > > drivers/target/iscsi/iscsi_target.c | 29 ++++++++++-------- > drivers/target/target_core_device.c | 19 +++++------- > drivers/target/target_core_tmr.c | 4 +-- > drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++-------- > drivers/target/target_core_xcopy.c | 9 ++++-- > drivers/usb/gadget/function/f_tcm.c | 6 ++-- > include/target/target_core_fabric.h | 9 +++--- > 7 files changed, 83 insertions(+), 48 deletions(-) > Reviewed-by: Mike Christie <michael.christie@oracle.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 0/4] target: fix NULL pointer dereference @ 2020-06-07 20:37 ` Mike Christie 0 siblings, 0 replies; 22+ messages in thread From: Mike Christie @ 2020-06-07 20:37 UTC (permalink / raw) To: Sudhakar Panneerselvam, martin.petersen, target-devel, linux-scsi Cc: shirley.ma On 6/7/20 2:58 PM, Sudhakar Panneerselvam wrote: > The following set of commits address a NULL pointer dereference and some > refactoring around this issue. > > v4: > - initialize the LUN in transport_init_se_cmd() > > v3: > - fix NULL pointer dereference when cdb initialization fails > > v2: > - new helper is named as target_cmd_init_cdb() > - existing function, target_setup_cmd_from_cdb is renamed as > target_cmd_parse_cdb() > > Sudhakar Panneerselvam (4): > target: factor out a new helper, target_cmd_init_cdb() > target: initialize LUN in transport_init_se_cmd(). > target: fix NULL pointer dereference > target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() > > drivers/target/iscsi/iscsi_target.c | 29 ++++++++++-------- > drivers/target/target_core_device.c | 19 +++++------- > drivers/target/target_core_tmr.c | 4 +-- > drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++-------- > drivers/target/target_core_xcopy.c | 9 ++++-- > drivers/usb/gadget/function/f_tcm.c | 6 ++-- > include/target/target_core_fabric.h | 9 +++--- > 7 files changed, 83 insertions(+), 48 deletions(-) > Reviewed-by: Mike Christie <michael.christie@oracle.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 0/4] target: fix NULL pointer dereference 2020-06-07 19:58 ` Sudhakar Panneerselvam @ 2020-06-10 2:02 ` Martin K. Petersen -1 siblings, 0 replies; 22+ messages in thread From: Martin K. Petersen @ 2020-06-10 2:02 UTC (permalink / raw) To: target-devel, michael.christie, Sudhakar Panneerselvam, linux-scsi Cc: Martin K . Petersen, shirley.ma On Sun, 7 Jun 2020 19:58:29 +0000, Sudhakar Panneerselvam wrote: > The following set of commits address a NULL pointer dereference and some > refactoring around this issue. > > v4: > - initialize the LUN in transport_init_se_cmd() > > v3: > - fix NULL pointer dereference when cdb initialization fails > > [...] Applied to 5.8/scsi-queue, thanks! [1/4] scsi: target: Factor out a new helper, target_cmd_init_cdb() https://git.kernel.org/mkp/scsi/c/f98c2ddf8ba3 [2/4] scsi: target: Initialize LUN in transport_init_se_cmd() https://git.kernel.org/mkp/scsi/c/a36840d80027 [3/4] scsi: target: Fix NULL pointer dereference https://git.kernel.org/mkp/scsi/c/9e95fb805dc0 [4/4] scsi: target: Rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() https://git.kernel.org/mkp/scsi/c/987db58737e2 -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 0/4] target: fix NULL pointer dereference @ 2020-06-10 2:02 ` Martin K. Petersen 0 siblings, 0 replies; 22+ messages in thread From: Martin K. Petersen @ 2020-06-10 2:02 UTC (permalink / raw) To: target-devel, michael.christie, Sudhakar Panneerselvam, linux-scsi Cc: Martin K . Petersen, shirley.ma On Sun, 7 Jun 2020 19:58:29 +0000, Sudhakar Panneerselvam wrote: > The following set of commits address a NULL pointer dereference and some > refactoring around this issue. > > v4: > - initialize the LUN in transport_init_se_cmd() > > v3: > - fix NULL pointer dereference when cdb initialization fails > > [...] Applied to 5.8/scsi-queue, thanks! [1/4] scsi: target: Factor out a new helper, target_cmd_init_cdb() https://git.kernel.org/mkp/scsi/c/f98c2ddf8ba3 [2/4] scsi: target: Initialize LUN in transport_init_se_cmd() https://git.kernel.org/mkp/scsi/c/a36840d80027 [3/4] scsi: target: Fix NULL pointer dereference https://git.kernel.org/mkp/scsi/c/9e95fb805dc0 [4/4] scsi: target: Rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() https://git.kernel.org/mkp/scsi/c/987db58737e2 -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-09-03 14:52 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-07 19:58 [PATCH v4 0/4] target: fix NULL pointer dereference Sudhakar Panneerselvam 2020-06-07 19:58 ` Sudhakar Panneerselvam 2020-06-07 19:58 ` [PATCH v4 1/4] target: factor out a new helper, target_cmd_init_cdb() Sudhakar Panneerselvam 2020-06-07 19:58 ` Sudhakar Panneerselvam 2020-06-07 19:58 ` [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd() Sudhakar Panneerselvam 2020-06-07 19:58 ` Sudhakar Panneerselvam 2020-09-02 13:49 ` Martin Wilck 2020-09-02 13:49 ` Martin Wilck 2020-09-02 15:14 ` Sudhakar Panneerselvam 2020-09-02 15:14 ` Sudhakar Panneerselvam 2020-09-03 13:00 ` Martin Wilck 2020-09-03 13:00 ` Martin Wilck 2020-09-03 13:13 ` Martin Wilck 2020-09-03 13:13 ` Martin Wilck 2020-06-07 19:58 ` [PATCH v4 3/4] target: fix NULL pointer dereference Sudhakar Panneerselvam 2020-06-07 19:58 ` Sudhakar Panneerselvam 2020-06-07 19:58 ` [PATCH v4 4/4] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() Sudhakar Panneerselvam 2020-06-07 19:58 ` Sudhakar Panneerselvam 2020-06-07 20:37 ` [PATCH v4 0/4] target: fix NULL pointer dereference Mike Christie 2020-06-07 20:37 ` Mike Christie 2020-06-10 2:02 ` Martin K. Petersen 2020-06-10 2:02 ` Martin K. Petersen
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.