From 7a4e9129fc5016c0046d29b4e851f60f086bd522 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Thu, 15 Jan 2009 18:50:43 +0000 Subject: [PATCH] - Minor optimization - New member target_name added to struct scst_user_sess - Docs updates git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@634 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- doc/scst_user_spec.txt | 15 ++++--- scst/README | 23 ++++------ scst/include/scst_const.h | 5 +++ scst/include/scst_user.h | 7 +-- scst/src/dev_handlers/scst_user.c | 75 +++++++++++++++++++++++++------ scst/src/scst_priv.h | 3 -- usr/fileio/common.c | 7 +-- 7 files changed, 90 insertions(+), 45 deletions(-) diff --git a/doc/scst_user_spec.txt b/doc/scst_user_spec.txt index e1fd9071c..ceeb4ade8 100644 --- a/doc/scst_user_spec.txt +++ b/doc/scst_user_spec.txt @@ -206,10 +206,10 @@ which is defined as the following: struct scst_user_get_cmd { - uint64_t preply; uint32_t cmd_h; uint32_t subcode; union { + uint64_t preply; struct scst_user_sess sess; struct scst_user_scsi_cmd_parse parse_cmd; struct scst_user_scsi_cmd_alloc_mem alloc_cmd; @@ -222,15 +222,15 @@ struct scst_user_get_cmd where: - - preply - pointer to the reply data or, if 0, there is no reply. See - SCST_USER_REPLY_CMD for description of struct scst_user_reply_cmd - fields - - cmd_h - command handle used to identify the command in the reply. - subcode - subcommand code, see 4.1 below -Union contains command's specific payload. + - preply - pointer to the reply data or, if 0, there is no reply. See + SCST_USER_REPLY_CMD for description of struct scst_user_reply_cmd + fields + +Other union members contain command's specific payload. For all received subcommands the user space device handler shall call SCST_USER_REPLY_AND_GET_CMD or SCST_USER_REPLY_CMD function to tell SCST @@ -254,6 +254,7 @@ struct scst_user_sess uint16_t threads_num; uint8_t rd_only; char initiator_name[SCST_MAX_NAME]; + char target_name[SCST_MAX_NAME]; }, where: @@ -270,6 +271,8 @@ where: - initiator_name - name of the remote initiator, which initiated this session + - target_name - name of the target, to which this session belongs + When SCST_USER_ATTACH_SESS is returned, it is guaranteed that there are no other commands are being executed or pending. diff --git a/scst/README b/scst/README index f5f9c770a..c686b6ccf 100644 --- a/scst/README +++ b/scst/README @@ -470,7 +470,7 @@ target mode in it, as for qla2x00t driver, it will immediately start accepting new connections, hence creating new sessions, and those new sessions will be assigned to security groups according to the *currently* configured access control settings. For instance, to -"Default" group, instead of "HOST004" as you need, because "HOST004" +"Default" group, instead of "HOST004" as you may need, because "HOST004" doesn't exist yet. So, one must configure all the security groups before new connections from the initiators are created, i.e. before target drivers loaded. @@ -904,7 +904,7 @@ By default, this timeout is 30 or 60 seconds, depending on your distribution. 4. Try to avoid such seek intensive workloads. -5. increase speed of the target's backstorage. +5. Increase speed of the target's backstorage. 6. Implement in SCST dynamic I/O flow control. This will be an ultimate solution. See "Dynamic I/O flow control" section on @@ -919,18 +919,13 @@ command, hence one or more commands in the tail of the queue can not be served on time less than the timeout, so the initiator will decide that they are stuck on the target and will try to recover. -Unfortunately, target can reliably detect leading to the issue -conditions only in case of READ commands, when the target can see that -commands' data transmission is getting too slow, so the dynamic flow -control, described above, can prevent the issue. But for WRITE commands -there are cases when target has no way to detect the issue. In this case -you can workaround it only by increasing the corresponding timeout on -the initiator. - -Thus, to workaround/fix this issue in this case you can use ways 1, 2, -3, 6 above or (7) increase speed of the link between target and -initiator. But for write intensive workloads you may have to increase -the timeout on initiator (way 3) in any case. +To workaround/fix this issue in this case you can use ways 1, 2, 3, 6 +above or (7): increase speed of the link between target and initiator. +But for some initiators implementations for WRITE commands there might +be cases when target has no way to detect the issue, so dynamic I/O flow +control will not be able to help. In those cases you could also need on +the initiator(s) to either decrease the queue depth (way 2), or increase +the corresponding timeout (way 3). Note, that logged messages about QUEUE_FULL status are quite different by nature. This is a normal work, just SCSI flow control in action. diff --git a/scst/include/scst_const.h b/scst/include/scst_const.h index 3f7fea259..e2be7b8f3 100644 --- a/scst/include/scst_const.h +++ b/scst/include/scst_const.h @@ -109,6 +109,11 @@ enum scst_cmd_queue_type { #define SCST_DATA_READ 2 #define SCST_DATA_NONE 3 +/************************************************************* + ** Name of the "Default" security group + *************************************************************/ +#define SCST_DEFAULT_ACG_NAME "Default" + /************************************************************* ** Sense manipulation and examination *************************************************************/ diff --git a/scst/include/scst_user.h b/scst/include/scst_user.h index f89630d95..6a72b85bb 100644 --- a/scst/include/scst_user.h +++ b/scst/include/scst_user.h @@ -30,11 +30,7 @@ #define DEV_USER_VERSION \ DEV_USER_VERSION_NAME "$Revision$" SCST_CONST_VERSION -/* - * Chosen so sizeof(scst_user_sess) <= sizeof(scst_user_scsi_cmd_exec) - * (the largest one) - */ -#define SCST_MAX_NAME 45 +#define SCST_MAX_NAME 50 #define SCST_USER_PARSE_STANDARD 0 #define SCST_USER_PARSE_CALL 1 @@ -106,6 +102,7 @@ struct scst_user_sess { uint16_t threads_num; uint8_t rd_only; char initiator_name[SCST_MAX_NAME]; + char target_name[SCST_MAX_NAME]; }; struct scst_user_scsi_cmd_parse { diff --git a/scst/src/dev_handlers/scst_user.c b/scst/src/dev_handlers/scst_user.c index d820aab25..d57271d59 100644 --- a/scst/src/dev_handlers/scst_user.c +++ b/scst/src/dev_handlers/scst_user.c @@ -20,6 +20,7 @@ #include #include #include +#include #define LOG_PREFIX DEV_USER_NAME @@ -53,11 +54,11 @@ #endif #define DEV_USER_MAJOR 237 - #define DEV_USER_CMD_HASH_ORDER 6 - #define DEV_USER_ATTACH_TIMEOUT (5*HZ) +#define DEV_USER_HEADER_LEN offsetof(struct scst_user_get_cmd, preply) + struct scst_user_dev { struct rw_semaphore dev_rwsem; @@ -144,6 +145,7 @@ struct scst_user_cmd { unsigned int h; struct list_head hash_list_entry; + int user_cmd_payload_len; struct scst_user_get_cmd user_cmd; /* cmpl used only by ATTACH_SESS, mcmd used only by TM */ @@ -410,6 +412,7 @@ static void dev_user_on_cached_mem_free(struct scst_user_cmd *ucmd) TRACE_MEM("Preparing ON_CACHED_MEM_FREE (ucmd %p, h %d, ubuff %lx)", ucmd, ucmd->h, ucmd->ubuff); + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.on_cached_mem_free); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_ON_CACHED_MEM_FREE; ucmd->user_cmd.on_cached_mem_free.pbuf = ucmd->ubuff; @@ -627,6 +630,7 @@ static int dev_user_alloc_space(struct scst_user_cmd *ucmd) goto out; } + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.alloc_cmd); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_ALLOC_MEM; ucmd->user_cmd.alloc_cmd.sess_h = (unsigned long)cmd->tgt_dev; @@ -747,6 +751,7 @@ static int dev_user_parse(struct scst_cmd *cmd) case SCST_USER_PARSE_CALL: TRACE_DBG("Preparing PARSE for user space (ucmd=%p, h=%d, " "bufflen %d)", ucmd, ucmd->h, cmd->bufflen); + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.parse_cmd); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_PARSE; ucmd->user_cmd.parse_cmd.sess_h = (unsigned long)cmd->tgt_dev; @@ -858,6 +863,7 @@ static int dev_user_exec(struct scst_cmd *cmd) if (cmd->data_direction == SCST_DATA_WRITE) dev_user_flush_dcache(ucmd); + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.exec_cmd); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_EXEC; ucmd->user_cmd.exec_cmd.sess_h = (unsigned long)cmd->tgt_dev; @@ -926,9 +932,9 @@ static void dev_user_on_free_cmd(struct scst_cmd *cmd) goto out_reply; } + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.on_free_cmd); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_ON_FREE_CMD; - ucmd->user_cmd.on_free_cmd.pbuf = ucmd->ubuff; ucmd->user_cmd.on_free_cmd.resp_data_len = cmd->resp_data_len; ucmd->user_cmd.on_free_cmd.buffer_cached = ucmd->buff_cached; @@ -1712,18 +1718,44 @@ static int dev_user_reply_get_cmd(struct file *file, void __user *arg) goto out_free; } + kmem_cache_free(user_get_cmd_cachep, cmd); + spin_lock_irq(&dev->cmd_lists.cmd_list_lock); res = dev_user_get_next_cmd(dev, &ucmd); if (res == 0) { - *cmd = ucmd->user_cmd; - spin_unlock_irq(&dev->cmd_lists.cmd_list_lock); - TRACE_BUFFER("UCMD", cmd, sizeof(*cmd)); - res = copy_to_user(arg, cmd, sizeof(*cmd)); - } else + int len; + /* + * A misbehaving user space handler can make ucmd to get dead + * immediately after we released the lock, which can lead to + * copy of dead data to the user space, which can lead to a + * leak of sensitive information. + */ + ucmd_get(ucmd); spin_unlock_irq(&dev->cmd_lists.cmd_list_lock); -out_free: - kmem_cache_free(user_get_cmd_cachep, cmd); + EXTRACHECKS_BUG_ON(ucmd->user_cmd_payload_len == 0); + + len = DEV_USER_HEADER_LEN + ucmd->user_cmd_payload_len; + TRACE_DBG("ucmd %p (user_cmd %p), payload_len %d (len %d)", + ucmd, &ucmd->user_cmd, ucmd->user_cmd_payload_len, len); + TRACE_BUFFER("UCMD", &ucmd->user_cmd, len); + res = copy_to_user(arg, &ucmd->user_cmd, len); + if (unlikely(res != 0)) { + /* Requeue ucmd back */ + TRACE_DBG("Copy to user failed (%d), requeuing ucmd %p " + "back to head of ready cmd list", res, ucmd); + spin_lock_irq(&dev->cmd_lists.cmd_list_lock); + list_add(&ucmd->ready_cmd_list_entry, + &dev->ready_cmd_list); + spin_unlock_irq(&dev->cmd_lists.cmd_list_lock); + } +#ifdef CONFIG_SCST_EXTRACHECKS + else + ucmd->user_cmd_payload_len = 0; +#endif + ucmd_put(ucmd); + } else + spin_unlock_irq(&dev->cmd_lists.cmd_list_lock); out_up: up_read(&dev->dev_rwsem); @@ -1731,6 +1763,10 @@ out_up: out: TRACE_EXIT_RES(res); return res; + +out_free: + kmem_cache_free(user_get_cmd_cachep, cmd); + goto out_up; } static long dev_user_ioctl(struct file *file, unsigned int cmd, @@ -2132,6 +2168,7 @@ static int dev_user_task_mgmt_fn(struct scst_mgmt_cmd *mcmd, /* We can't afford missing TM command due to memory shortage */ ucmd = dev_user_alloc_ucmd(dev, GFP_KERNEL|__GFP_NOFAIL); + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.tm_cmd); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_TASK_MGMT; ucmd->user_cmd.tm_cmd.sess_h = (unsigned long)tgt_dev; @@ -2266,6 +2303,7 @@ static int dev_user_attach_tgt(struct scst_tgt_dev *tgt_dev) ucmd->cmpl = &cmpl; + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.sess); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_ATTACH_SESS; ucmd->user_cmd.sess.sess_h = (unsigned long)tgt_dev; @@ -2277,12 +2315,20 @@ static int dev_user_attach_tgt(struct scst_tgt_dev *tgt_dev) sizeof(ucmd->user_cmd.sess.initiator_name)-1); ucmd->user_cmd.sess.initiator_name[ sizeof(ucmd->user_cmd.sess.initiator_name)-1] = '\0'; + if (tgt_dev->sess->tgt->default_group_name != NULL) { + strncpy(ucmd->user_cmd.sess.target_name, + &tgt_dev->sess->tgt->default_group_name[sizeof(SCST_DEFAULT_ACG_NAME)], + sizeof(ucmd->user_cmd.sess.target_name)-1); + ucmd->user_cmd.sess.target_name[ + sizeof(ucmd->user_cmd.sess.target_name)-1] = '\0'; + } TRACE_MGMT_DBG("Preparing ATTACH_SESS %p (h %d, sess_h %llx, LUN %llx, " - "threads_num %d, rd_only_flag %d, initiator %s)", ucmd, ucmd->h, - ucmd->user_cmd.sess.sess_h, ucmd->user_cmd.sess.lun, - ucmd->user_cmd.sess.threads_num, ucmd->user_cmd.sess.rd_only, - ucmd->user_cmd.sess.initiator_name); + "threads_num %d, rd_only_flag %d, initiator %s, target %s)", + ucmd, ucmd->h, ucmd->user_cmd.sess.sess_h, + ucmd->user_cmd.sess.lun, ucmd->user_cmd.sess.threads_num, + ucmd->user_cmd.sess.rd_only, ucmd->user_cmd.sess.initiator_name, + ucmd->user_cmd.sess.target_name); ucmd->state = UCMD_STATE_ATTACH_SESS; @@ -2334,6 +2380,7 @@ static void dev_user_detach_tgt(struct scst_tgt_dev *tgt_dev) TRACE_MGMT_DBG("Preparing DETACH_SESS %p (h %d, sess_h %llx)", ucmd, ucmd->h, ucmd->user_cmd.sess.sess_h); + ucmd->user_cmd_payload_len = sizeof(ucmd->user_cmd.sess); ucmd->user_cmd.cmd_h = ucmd->h; ucmd->user_cmd.subcode = SCST_USER_DETACH_SESS; ucmd->user_cmd.sess.sess_h = (unsigned long)tgt_dev; diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index b14dbf989..e1cd186b5 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -107,9 +107,6 @@ extern unsigned long scst_trace_flag; #define SCST_CMD_STATE_RES_CONT_SAME SCST_EXEC_NOT_COMPLETED #define SCST_CMD_STATE_RES_NEED_THREAD SCST_EXEC_NEED_THREAD -/** Name of the "default" security group **/ -#define SCST_DEFAULT_ACG_NAME "Default" - /** ** Maximum count of uncompleted commands that an initiator could ** queue on any device. Then it will start getting TASK QUEUE FULL status. diff --git a/usr/fileio/common.c b/usr/fileio/common.c index 50d0e9bdf..d65ce7067 100644 --- a/usr/fileio/common.c +++ b/usr/fileio/common.c @@ -618,9 +618,10 @@ static int do_sess(struct vdisk_cmd *vcmd) tgt_dev->sess_h = cmd->sess.sess_h; tgt_dev->last_write_cmd_queue_type = SCST_CMD_QUEUE_SIMPLE; - PRINT_INFO("Session from initiator %s attached (LUN %"PRIx64", " - "threads_num %d, rd_only %d, sess_h %"PRIx64")", - cmd->sess.initiator_name, cmd->sess.lun, + PRINT_INFO("Session from initiator %s (target %s) attached " + "(LUN %"PRIx64", threads_num %d, rd_only %d, sess_h " + "%"PRIx64")", cmd->sess.initiator_name, + cmd->sess.target_name, cmd->sess.lun, cmd->sess.threads_num, cmd->sess.rd_only, cmd->sess.sess_h); } else {