- 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
2026-05-14 09:11:27 +00:00 · 2009-01-15 18:50:43 +00:00
parent 5a9ff63133
commit 7a4e9129fc
7 changed files with 90 additions and 45 deletions
--- 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.
 
--- 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.
--- 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
 *************************************************************/
--- 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 {
--- a/scst/src/dev_handlers/scst_user.c
+++ b/scst/src/dev_handlers/scst_user.c
@@ -20,6 +20,7 @@
 #include <linux/kthread.h>
 #include <linux/delay.h>
 #include <linux/poll.h>
+#include <linux/stddef.h>

 #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;
--- 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.
--- 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 {