From e89cdc5bafaed258b30c98ee61561b557dbdaefe Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Mon, 10 Mar 2008 10:59:56 +0000 Subject: [PATCH] - Fixed READ POSITION command handling - Fixed race in TM handling git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@299 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scsi_tgt.h | 7 +- scst/src/scst_cdbprobe.h | 21 +++-- scst/src/scst_lib.c | 191 +++++++++++++++++++++++++-------------- scst/src/scst_targ.c | 15 +-- 4 files changed, 143 insertions(+), 91 deletions(-) diff --git a/scst/include/scsi_tgt.h b/scst/include/scsi_tgt.h index 104a939de..d2eb99f4c 100644 --- a/scst/include/scsi_tgt.h +++ b/scst/include/scsi_tgt.h @@ -1284,7 +1284,6 @@ struct scst_mgmt_cmd int fn; unsigned int completed:1; /* set, if the mcmd is completed */ - unsigned int active:1; /* set, if the mcmd is active */ /* Set if device(s) should be unblocked after mcmd's finish */ unsigned int needs_unblocking:1; unsigned int lun_set:1; /* set, if lun field is valid */ @@ -1884,11 +1883,9 @@ static inline int scst_rx_mgmt_fn_lun(struct scst_session *sess, int fn, * cdb_p - pointer to CDB * dev_type - SCSI device type * op_flags, direction, transfer_len, cdb_len, op_name - the result (output) - * Returns 0 on success, -1 otherwise + * Returns: 0 on success, <0 if command is unknown, >0 if command is invalid. */ -int scst_get_cdb_info(const uint8_t *cdb_p, int dev_type, - enum scst_cdb_flags *op_flags, scst_data_direction *direction, - unsigned int *transfer_len, int *cdb_len, const char **op_name); +int scst_get_cdb_info(struct scst_cmd *cmd); /* * Set error SCSI status in the command and prepares it for returning it diff --git a/scst/src/scst_cdbprobe.h b/scst/src/scst_cdbprobe.h index 426713888..0c2a7035e 100644 --- a/scst/src/scst_cdbprobe.h +++ b/scst/src/scst_cdbprobe.h @@ -21,16 +21,17 @@ #define __SCST_CDBPROBE_H /* get_trans_len_x extract x bytes from cdb as length starting from off */ -static uint32_t get_trans_len_1(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_2(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_3(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_4(const uint8_t *cdb, uint8_t off); +static int get_trans_len_1(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_2(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_3(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_4(struct scst_cmd *cmd, uint8_t off); /* for special commands */ -static uint32_t get_trans_len_block_limit(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_read_capacity(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_single(const uint8_t *cdb, uint8_t off); -static uint32_t get_trans_len_none(const uint8_t *cdb, uint8_t off); +static int get_trans_len_block_limit(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_read_capacity(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_single(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_none(struct scst_cmd *cmd, uint8_t off); +static int get_trans_len_read_pos(struct scst_cmd *cmd, uint8_t off); /* +=====================================-============-======- @@ -80,7 +81,7 @@ struct scst_sdbops */ uint8_t flags; /* opcode -- various flags */ uint8_t off; /* length offset in cdb */ - uint32_t (*get_trans_len)(const uint8_t *cdb, uint8_t off) __attribute__ ((aligned)); + int (*get_trans_len)(struct scst_cmd *cmd, uint8_t off) __attribute__ ((aligned)); } __attribute__((packed)); static int scst_scsi_op_list[256]; @@ -252,7 +253,7 @@ static const struct scst_sdbops scst_scsi_op_table[] = { {0x33, "O OO O ", "SET LIMITS(10)", SCST_DATA_NONE, FLAG_NONE, 0, get_trans_len_none}, {0x34, " O ", "READ POSITION", - SCST_DATA_READ, SCST_SMALL_TIMEOUT, 7, get_trans_len_2}, + SCST_DATA_READ, SCST_SMALL_TIMEOUT, 7, get_trans_len_read_pos}, {0x34, " O ", "GET DATA BUFFER STATUS", SCST_DATA_READ, FLAG_NONE, 7, get_trans_len_2}, {0x34, "O OO O ", "PRE-FETCH", diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 1039c41e0..39f448770 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -1650,79 +1650,132 @@ int scst_get_cdb_len(const uint8_t *cdb) /* get_trans_len_x extract x bytes from cdb as length starting from off */ -static uint32_t get_trans_len_1(const uint8_t *cdb, uint8_t off) -{ - u32 len; - - len = (u32)cdb[off]; - return len; -} - -static uint32_t get_trans_len_2(const uint8_t *cdb, uint8_t off) -{ - const uint8_t *p = cdb + off; - u32 len = 0; - - len |= ((u32)p[0]) << 8; - len |= ((u32)p[1]); - return len; -} - -static uint32_t get_trans_len_3(const uint8_t *cdb, uint8_t off) -{ - const uint8_t *p = cdb + off; - u32 len = 0; - - len |= ((u32)p[0]) << 16; - len |= ((u32)p[1]) << 8; - len |= ((u32)p[2]); - return len; -} - -static uint32_t get_trans_len_4(const uint8_t *cdb, uint8_t off) -{ - const uint8_t *p = cdb + off; - u32 len = 0; - - len |= ((u32)p[0]) << 24; - len |= ((u32)p[1]) << 16; - len |= ((u32)p[2]) << 8; - len |= ((u32)p[3]); - return len; -} - /* for special commands */ -static uint32_t get_trans_len_block_limit(const uint8_t *cdb, uint8_t off) -{ - return 6; -} - -static uint32_t get_trans_len_read_capacity(const uint8_t *cdb, uint8_t off) -{ - return READ_CAP_LEN; -} - -static uint32_t get_trans_len_single(const uint8_t *cdb, uint8_t off) -{ - return 1; -} - -static uint32_t get_trans_len_none(const uint8_t *cdb, uint8_t off) +static int get_trans_len_block_limit(struct scst_cmd *cmd, uint8_t off) { + cmd->bufflen = 6; return 0; } -int scst_get_cdb_info(const uint8_t *cdb_p, int dev_type, - enum scst_cdb_flags *op_flags, scst_data_direction *direction, - unsigned int *transfer_len, int *cdb_len, const char **op_name) +static int get_trans_len_read_capacity(struct scst_cmd *cmd, uint8_t off) { + cmd->bufflen = READ_CAP_LEN; + return 0; +} + +static int get_trans_len_single(struct scst_cmd *cmd, uint8_t off) +{ + cmd->bufflen = 1; + return 0; +} + +static int get_trans_len_read_pos(struct scst_cmd *cmd, uint8_t off) +{ + uint8_t *p = (uint8_t *)cmd->cdb + off; + int res = 0; + + cmd->bufflen = 0; + cmd->bufflen |= ((u32)p[0]) << 8; + cmd->bufflen |= ((u32)p[1]); + + switch (cmd->cdb[1] & 0x1f) { + case 0: + case 1: + case 6: + if (cmd->bufflen != 0) { + PRINT_ERROR("READ POSITION: Invalid non-zero (%d) " + "allocation length for service action %x", + cmd->bufflen, cmd->cdb[1] & 0x1f); + goto out_inval; + } + break; + } + + switch (cmd->cdb[1] & 0x1f) { + case 0: + case 1: + cmd->bufflen = 20; + break; + case 6: + cmd->bufflen = 32; + break; + case 8: + cmd->bufflen = max(28, cmd->bufflen); + break; + default: + PRINT_ERROR("READ POSITION: Invalid service action %x", + cmd->cdb[1] & 0x1f); + goto out_inval; + } + +out: + return res; + +out_inval: + scst_set_cmd_error(cmd, + SCST_LOAD_SENSE(scst_sense_invalid_field_in_cdb)); + res = 1; + goto out; +} + +static int get_trans_len_1(struct scst_cmd *cmd, uint8_t off) +{ + cmd->bufflen = (u32)cmd->cdb[off]; + return 0; +} + +static int get_trans_len_2(struct scst_cmd *cmd, uint8_t off) +{ + const uint8_t *p = cmd->cdb + off; + + cmd->bufflen = 0; + cmd->bufflen |= ((u32)p[0]) << 8; + cmd->bufflen |= ((u32)p[1]); + + return 0; +} + +static int get_trans_len_3(struct scst_cmd *cmd, uint8_t off) +{ + const uint8_t *p = cmd->cdb + off; + + cmd->bufflen = 0; + cmd->bufflen |= ((u32)p[0]) << 16; + cmd->bufflen |= ((u32)p[1]) << 8; + cmd->bufflen |= ((u32)p[2]); + + return 0; +} + +static int get_trans_len_4(struct scst_cmd *cmd, uint8_t off) +{ + const uint8_t *p = cmd->cdb + off; + + cmd->bufflen = 0; + cmd->bufflen |= ((u32)p[0]) << 24; + cmd->bufflen |= ((u32)p[1]) << 16; + cmd->bufflen |= ((u32)p[2]) << 8; + cmd->bufflen |= ((u32)p[3]); + + return 0; +} + +static int get_trans_len_none(struct scst_cmd *cmd, uint8_t off) +{ + cmd->bufflen = 0; + return 0; +} + +int scst_get_cdb_info(struct scst_cmd *cmd) +{ + int dev_type = cmd->dev->handler->type; int i, res = 0; uint8_t op; const struct scst_sdbops *ptr = NULL; TRACE_ENTRY(); - op = *cdb_p; /* get clear opcode */ + op = cmd->cdb[0]; /* get clear opcode */ TRACE_DBG("opcode=%02x, cdblen=%d bytes, tblsize=%d, " "dev_type=%d", op, SCST_GET_CDB_LEN(op), SCST_CDB_TBL_SIZE, @@ -1758,15 +1811,15 @@ int scst_get_cdb_info(const uint8_t *cdb_p, int dev_type, TRACE(TRACE_SCSI, "Unknown opcode 0x%x for type %d", op, dev_type); res = -1; - *op_flags = SCST_INFO_INVALID; + cmd->op_flags = SCST_INFO_INVALID; goto out; } - *cdb_len = SCST_GET_CDB_LEN(op); - *op_name = ptr->op_name; - *direction = ptr->direction; - *op_flags = ptr->flags; - *transfer_len = (*ptr->get_trans_len)(cdb_p, ptr->off); + cmd->cdb_len = SCST_GET_CDB_LEN(op); + cmd->op_name = ptr->op_name; + cmd->data_direction = ptr->direction; + cmd->op_flags = ptr->flags; + res = (*ptr->get_trans_len)(cmd, ptr->off); out: TRACE_EXIT(); @@ -1788,7 +1841,7 @@ lun_t scst_unpack_lun(const uint8_t *lun, int len) TRACE_BUFF_FLAG(TRACE_DEBUG, "Raw LUN", lun, len); - if (len < 2) { + if (unlikely(len < 2)) { PRINT_ERROR("Illegal lun length %d, expected 2 bytes or " "more", len); goto out; @@ -1797,12 +1850,10 @@ lun_t scst_unpack_lun(const uint8_t *lun, int len) if (len > 2) { switch(len) { case 8: - { if ((*((uint64_t*)lun) & __constant_cpu_to_be64(0x0000FFFFFFFFFFFFLL)) != 0) goto out_err; break; - } case 4: if (*((uint16_t*)&lun[2]) != 0) goto out_err; diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 6ed0a7590..2f4803658 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -316,6 +316,7 @@ static int scst_pre_parse(struct scst_cmd *cmd) { int res = SCST_CMD_STATE_RES_CONT_SAME; struct scst_device *dev = cmd->dev; + int rc; TRACE_ENTRY(); @@ -333,12 +334,16 @@ static int scst_pre_parse(struct scst_cmd *cmd) * transfer data. */ - if (unlikely(scst_get_cdb_info(cmd->cdb, dev->handler->type, - &cmd->op_flags, &cmd->data_direction, - &cmd->bufflen, &cmd->cdb_len, &cmd->op_name) != 0)) { + rc = scst_get_cdb_info(cmd); + if (unlikely(rc != 0)) { + if (rc > 0) { + PRINT_BUFFER("Failed CDB", cmd->cdb, cmd->cdb_len); + goto out_xmit; + } PRINT_ERROR("Unknown opcode 0x%02x for %s. " "Should you update scst_scsi_op_table?", cmd->cdb[0], dev->handler->name); + PRINT_BUFFER("Failed CDB", cmd->cdb, cmd->cdb_len); #ifdef USE_EXPECTED_VALUES if (scst_cmd_is_expected_set(cmd)) { TRACE(TRACE_SCSI, "Using initiator supplied values: " @@ -3615,7 +3620,6 @@ static int scst_set_mcmd_next_state(struct scst_mgmt_cmd *mcmd) res = -1; } - mcmd->nexus_loss_check_active = 0; mcmd->completed = 1; out_unlock: @@ -4349,9 +4353,8 @@ static int scst_mgmt_cmd_check_nexus_loss(struct scst_mgmt_cmd *mcmd) sess->unreg_cmds_done_fn = NULL; } - spin_lock_irq(&scst_mcmd_lock); mcmd->nexus_loss_check_done = 1; - spin_unlock_irq(&scst_mcmd_lock); + mcmd->nexus_loss_check_active = 0; res = scst_set_mcmd_next_state(mcmd);