[PATCH 0/5] target: Bugfixes for v3.1-rc6

[PATCH 0/5] target: Bugfixes for v3.1-rc6

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi Folks,

The following series contains the (hopefully) last set of target v3.1-rc6
bugfixes to address outstanding regressions, and a handful other issues
related to v3.1 mainline target core + iscsi-target code.  This includes:

*) A bugfix from Christoph to use a work queue to address breakage in the
   current tcm_fc(fcoe) I/O path
*) A bugfix from Kiran to fix tcm_fc(fcoe) DDP offload error conditions
*) A bugfix from Roland to address list corruption in target core handling of
   QUEUE_FULL status.
*) A change to use only hex characters in the binary conversion of
   vpd_unit_serial for EVPD=0x83 NAA IEEE Registered Extended in target core
   to address a bug with internal hex2bin() usage, and finally,
*) A change to explictly disable iSCSI marker parameter key usage in order to
   remove a potentially dangerous local scope array allocation with markers
   enabled in the main iscsi-target I/O path.

Please go ahead and have a final look, and if there are no complaints they will
be headed to Linus in [GIT PULL] format soon to make next weeks -rc7 / -final
release.
 
This can be pulled directly from a temporary new target-pending.git tree based
upon the latest github v3.1-rc6 while kernel.org infrastructure is down:

    git://linux-iscsi.org/target-pending.git 3.1-rc-fixes

Thank you,

--nab

Christoph Hellwig (1):
  tcm_fc: Work queue based approach instead of managing own thread and
    event based mechanism

Kiran Patil (1):
  tcm_fc: Invalidation of DDP context for FCoE target in error
    conditions

Nicholas Bellinger (2):
  target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered
    Extended
  iscsi-target: Disable markers + remove dangerous local scope array
    usage

Roland Dreier (1):
  target: Fix race between multiple invocations of target_qf_do_work()

 drivers/target/iscsi/iscsi_target_parameters.c |    2 +-
 drivers/target/iscsi/iscsi_target_util.c       |  248 +-----------------------
 drivers/target/target_core_cdb.c               |   35 +++-
 drivers/target/target_core_transport.c         |    9 +-
 drivers/target/tcm_fc/tcm_fc.h                 |   12 +-
 drivers/target/tcm_fc/tfc_cmd.c                |   90 +--------
 drivers/target/tcm_fc/tfc_conf.c               |    7 +-
 drivers/target/tcm_fc/tfc_io.c                 |   62 +++---
 8 files changed, 90 insertions(+), 375 deletions(-)

[PATCH 1/5] target: Fix race between multiple invocations of target_qf_do_work()

[Nicholas A. Bellinger]

From: Roland Dreier <roland@purestorage.com>

When work is scheduled with schedule_work(), the work can end up
running on multiple CPUs at the same time -- this happens if
the work is already running on one CPU and schedule_work() is called
on another CPU.  This leads to list corruption with target_qf_do_work(),
which is roughly doing:

	spin_lock(...);
	list_for_each_entry_safe(...) {
		list_del(...);
		spin_unlock(...);

		// do stuff

		spin_lock(...);
	}

With multiple CPUs running this code, one CPU can end up deleting the
list entry that the other CPU is about to work on.

Fix this by splicing the list entries onto a local list and then
operating on that in the work function.  This way, each invocation of
target_qf_do_work() operates on its own local list and so multiple
invocations don't corrupt each other's list.  This also avoids dropping
and reacquiring the lock for each list entry.

Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8d0c58e..a4b0a8d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -977,15 +977,17 @@ static void target_qf_do_work(struct work_struct *work)
 {
 	struct se_device *dev = container_of(work, struct se_device,
 					qf_work_queue);
+	LIST_HEAD(qf_cmd_list);
 	struct se_cmd *cmd, *cmd_tmp;
 
 	spin_lock_irq(&dev->qf_cmd_lock);
-	list_for_each_entry_safe(cmd, cmd_tmp, &dev->qf_cmd_list, se_qf_node) {
+	list_splice_init(&dev->qf_cmd_list, &qf_cmd_list);
+	spin_unlock_irq(&dev->qf_cmd_lock);
 
+	list_for_each_entry_safe(cmd, cmd_tmp, &qf_cmd_list, se_qf_node) {
 		list_del(&cmd->se_qf_node);
 		atomic_dec(&dev->dev_qf_count);
 		smp_mb__after_atomic_dec();
-		spin_unlock_irq(&dev->qf_cmd_lock);
 
 		pr_debug("Processing %s cmd: %p QUEUE_FULL in work queue"
 			" context: %s\n", cmd->se_tfo->get_fabric_name(), cmd,
@@ -997,10 +999,7 @@ static void target_qf_do_work(struct work_struct *work)
 		 * has been added to head of queue
 		 */
 		transport_add_cmd_to_queue(cmd, cmd->t_state);
-
-		spin_lock_irq(&dev->qf_cmd_lock);
 	}
-	spin_unlock_irq(&dev->qf_cmd_lock);
 }
 
 unsigned char *transport_dump_cmd_direction(struct se_cmd *cmd)
-- 
1.5.6.5

[PATCH 2/5] tcm_fc: Invalidation of DDP context for FCoE target in error conditions

[Nicholas A. Bellinger]

From: Kiran Patil <kiran.patil@intel.com>

Problem: HW DDP context wasn;t invalidated in case of ABORTS, etc...
This leads to the problem where memory pages which are used for DDP
as user descriptor could get reused for some other purpose (such as to
satisfy new memory allocation request either by kernel or user mode threads)
and since HW DDP context was not invalidated, HW continue to write to
those pages, hence causing memory corruption.

Fix: Either on incoming ABORTS or due to exchange time out, allowed the
target to cleanup HW DDP context if it was setup for respective ft_cmd.
Added new function to perform this cleanup, furthur it can be enhanced
for other cleanup activity.

Additinal Notes: To avoid calling ddp_done from multiple places, composed
the functionality in helper function "ft_invl_hw_context" and it is being
called from multiple places. Cleaned up code in function "ft_recv_write_data"
w.r.t DDP.

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/tcm_fc/tfc_io.c |   62 +++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
index c37f4cd..d35ea5a 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -219,43 +219,41 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct fc_frame *fp)
 	if (cmd->was_ddp_setup) {
 		BUG_ON(!ep);
 		BUG_ON(!lport);
-	}
-
-	/*
-	 * Doesn't expect payload if DDP is setup. Payload
-	 * is expected to be copied directly to user buffers
-	 * due to DDP (Large Rx offload),
-	 */
-	buf = fc_frame_payload_get(fp, 1);
-	if (buf)
-		pr_err("%s: xid 0x%x, f_ctl 0x%x, cmd->sg %p, "
+		/*
+		 * Since DDP (Large Rx offload) was setup for this request,
+		 * payload is expected to be copied directly to user buffers.
+		 */
+		buf = fc_frame_payload_get(fp, 1);
+		if (buf)
+			pr_err("%s: xid 0x%x, f_ctl 0x%x, cmd->sg %p, "
 				"cmd->sg_cnt 0x%x. DDP was setup"
 				" hence not expected to receive frame with "
-				"payload, Frame will be dropped if "
-				"'Sequence Initiative' bit in f_ctl is "
+				"payload, Frame will be dropped if"
+				"'Sequence Initiative' bit in f_ctl is"
 				"not set\n", __func__, ep->xid, f_ctl,
 				cmd->sg, cmd->sg_cnt);
-	/*
- 	 * Invalidate HW DDP context if it was setup for respective
- 	 * command. Invalidation of HW DDP context is requited in both
- 	 * situation (success and error). 
- 	 */
-	ft_invl_hw_context(cmd);
+		/*
+		 * Invalidate HW DDP context if it was setup for respective
+		 * command. Invalidation of HW DDP context is requited in both
+		 * situation (success and error).
+		 */
+		ft_invl_hw_context(cmd);
 
-	/*
-	 * If "Sequence Initiative (TSI)" bit set in f_ctl, means last
-	 * write data frame is received successfully where payload is
-	 * posted directly to user buffer and only the last frame's
-	 * header is posted in receive queue.
-	 *
-	 * If "Sequence Initiative (TSI)" bit is not set, means error
-	 * condition w.r.t. DDP, hence drop the packet and let explict
-	 * ABORTS from other end of exchange timer trigger the recovery.
-	 */
-	if (f_ctl & FC_FC_SEQ_INIT)
-		goto last_frame;
-	else
-		goto drop;
+		/*
+		 * If "Sequence Initiative (TSI)" bit set in f_ctl, means last
+		 * write data frame is received successfully where payload is
+		 * posted directly to user buffer and only the last frame's
+		 * header is posted in receive queue.
+		 *
+		 * If "Sequence Initiative (TSI)" bit is not set, means error
+		 * condition w.r.t. DDP, hence drop the packet and let explict
+		 * ABORTS from other end of exchange timer trigger the recovery.
+		 */
+		if (f_ctl & FC_FC_SEQ_INIT)
+			goto last_frame;
+		else
+			goto drop;
+	}
 
 	rel_off = ntohl(fh->fh_parm_offset);
 	frame_len = fr_len(fp);
-- 
1.5.6.5

[PATCH 3/5] tcm_fc: Work queue based approach instead of managing own thread and event based mechanism

[Nicholas A. Bellinger]

From: Christoph Hellwig <hch@lst.de>

Problem: Changed from wake_up_interruptible -> wake_up_process and
wait_event_interruptible-> schedule_timeout_interruptible broke the FCoE
target.  Earlier approach of wake_up_interruptible was also looking at
'queue_cnt' which is not necessary, because it increment of 'queue_cnt'
with wake_up_inetrriptible / waker_up_process introduces race condition.

Fix: Instead of fixing the code which used wake_up_process and remove
'queue_cnt', using work_queue based approach is cleaner and acheives
same result. As well, work queue based approach has less programming
overhead and OS manages threads which processes work queues.

This patch is developed by Christoph Hellwig and reviwed+validated by
Kiran Patil.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Cc: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/tcm_fc/tcm_fc.h   |   12 +----
 drivers/target/tcm_fc/tfc_cmd.c  |   90 +++++---------------------------------
 drivers/target/tcm_fc/tfc_conf.c |    7 +--
 3 files changed, 16 insertions(+), 93 deletions(-)

diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
index bd4fe21..3749d8b 100644
--- a/drivers/target/tcm_fc/tcm_fc.h
+++ b/drivers/target/tcm_fc/tcm_fc.h
@@ -98,8 +98,7 @@ struct ft_tpg {
 	struct list_head list;		/* linkage in ft_lport_acl tpg_list */
 	struct list_head lun_list;	/* head of LUNs */
 	struct se_portal_group se_tpg;
-	struct task_struct *thread;	/* processing thread */
-	struct se_queue_obj qobj;	/* queue for processing thread */
+	struct workqueue_struct *workqueue;
 };
 
 struct ft_lport_acl {
@@ -110,16 +109,10 @@ struct ft_lport_acl {
 	struct se_wwn fc_lport_wwn;
 };
 
-enum ft_cmd_state {
-	FC_CMD_ST_NEW = 0,
-	FC_CMD_ST_REJ
-};
-
 /*
  * Commands
  */
 struct ft_cmd {
-	enum ft_cmd_state state;
 	u32 lun;                        /* LUN from request */
 	struct ft_sess *sess;		/* session held for cmd */
 	struct fc_seq *seq;		/* sequence in exchange mgr */
@@ -127,7 +120,7 @@ struct ft_cmd {
 	struct fc_frame *req_frame;
 	unsigned char *cdb;		/* pointer to CDB inside frame */
 	u32 write_data_len;		/* data received on writes */
-	struct se_queue_req se_req;
+	struct work_struct work;
 	/* Local sense buffer */
 	unsigned char ft_sense_buffer[TRANSPORT_SENSE_BUFFER];
 	u32 was_ddp_setup:1;		/* Set only if ddp is setup */
@@ -177,7 +170,6 @@ int ft_is_state_remove(struct se_cmd *);
 /*
  * other internal functions.
  */
-int ft_thread(void *);
 void ft_recv_req(struct ft_sess *, struct fc_frame *);
 struct ft_tpg *ft_lport_find_tpg(struct fc_lport *);
 struct ft_node_acl *ft_acl_get(struct ft_tpg *, struct fc_rport_priv *);
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 5654dc2..80fbcde 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -62,8 +62,8 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
 	int count;
 
 	se_cmd = &cmd->se_cmd;
-	pr_debug("%s: cmd %p state %d sess %p seq %p se_cmd %p\n",
-		caller, cmd, cmd->state, cmd->sess, cmd->seq, se_cmd);
+	pr_debug("%s: cmd %p sess %p seq %p se_cmd %p\n",
+		caller, cmd, cmd->sess, cmd->seq, se_cmd);
 	pr_debug("%s: cmd %p cdb %p\n",
 		caller, cmd, cmd->cdb);
 	pr_debug("%s: cmd %p lun %d\n", caller, cmd, cmd->lun);
@@ -90,38 +90,6 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
 		16, 4, cmd->cdb, MAX_COMMAND_SIZE, 0);
 }
 
-static void ft_queue_cmd(struct ft_sess *sess, struct ft_cmd *cmd)
-{
-	struct ft_tpg *tpg = sess->tport->tpg;
-	struct se_queue_obj *qobj = &tpg->qobj;
-	unsigned long flags;
-
-	qobj = &sess->tport->tpg->qobj;
-	spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
-	list_add_tail(&cmd->se_req.qr_list, &qobj->qobj_list);
-	atomic_inc(&qobj->queue_cnt);
-	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
-
-	wake_up_process(tpg->thread);
-}
-
-static struct ft_cmd *ft_dequeue_cmd(struct se_queue_obj *qobj)
-{
-	unsigned long flags;
-	struct se_queue_req *qr;
-
-	spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
-	if (list_empty(&qobj->qobj_list)) {
-		spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
-		return NULL;
-	}
-	qr = list_first_entry(&qobj->qobj_list, struct se_queue_req, qr_list);
-	list_del(&qr->qr_list);
-	atomic_dec(&qobj->queue_cnt);
-	spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
-	return container_of(qr, struct ft_cmd, se_req);
-}
-
 static void ft_free_cmd(struct ft_cmd *cmd)
 {
 	struct fc_frame *fp;
@@ -282,9 +250,7 @@ u32 ft_get_task_tag(struct se_cmd *se_cmd)
 
 int ft_get_cmd_state(struct se_cmd *se_cmd)
 {
-	struct ft_cmd *cmd = container_of(se_cmd, struct ft_cmd, se_cmd);
-
-	return cmd->state;
+	return 0;
 }
 
 int ft_is_state_remove(struct se_cmd *se_cmd)
@@ -505,6 +471,8 @@ int ft_queue_tm_resp(struct se_cmd *se_cmd)
 	return 0;
 }
 
+static void ft_send_work(struct work_struct *work);
+
 /*
  * Handle incoming FCP command.
  */
@@ -523,7 +491,9 @@ static void ft_recv_cmd(struct ft_sess *sess, struct fc_frame *fp)
 		goto busy;
 	}
 	cmd->req_frame = fp;		/* hold frame during cmd */
-	ft_queue_cmd(sess, cmd);
+
+	INIT_WORK(&cmd->work, ft_send_work);
+	queue_work(sess->tport->tpg->workqueue, &cmd->work);
 	return;
 
 busy:
@@ -563,12 +533,13 @@ void ft_recv_req(struct ft_sess *sess, struct fc_frame *fp)
 /*
  * Send new command to target.
  */
-static void ft_send_cmd(struct ft_cmd *cmd)
+static void ft_send_work(struct work_struct *work)
 {
+	struct ft_cmd *cmd = container_of(work, struct ft_cmd, work);
 	struct fc_frame_header *fh = fc_frame_header_get(cmd->req_frame);
 	struct se_cmd *se_cmd;
 	struct fcp_cmnd *fcp;
-	int data_dir;
+	int data_dir = 0;
 	u32 data_len;
 	int task_attr;
 	int ret;
@@ -675,42 +646,3 @@ static void ft_send_cmd(struct ft_cmd *cmd)
 err:
 	ft_send_resp_code_and_free(cmd, FCP_CMND_FIELDS_INVALID);
 }
-
-/*
- * Handle request in the command thread.
- */
-static void ft_exec_req(struct ft_cmd *cmd)
-{
-	pr_debug("cmd state %x\n", cmd->state);
-	switch (cmd->state) {
-	case FC_CMD_ST_NEW:
-		ft_send_cmd(cmd);
-		break;
-	default:
-		break;
-	}
-}
-
-/*
- * Processing thread.
- * Currently one thread per tpg.
- */
-int ft_thread(void *arg)
-{
-	struct ft_tpg *tpg = arg;
-	struct se_queue_obj *qobj = &tpg->qobj;
-	struct ft_cmd *cmd;
-
-	while (!kthread_should_stop()) {
-		schedule_timeout_interruptible(MAX_SCHEDULE_TIMEOUT);
-		if (kthread_should_stop())
-			goto out;
-
-		cmd = ft_dequeue_cmd(qobj);
-		if (cmd)
-			ft_exec_req(cmd);
-	}
-
-out:
-	return 0;
-}
diff --git a/drivers/target/tcm_fc/tfc_conf.c b/drivers/target/tcm_fc/tfc_conf.c
index b15879d..8fa39b7 100644
--- a/drivers/target/tcm_fc/tfc_conf.c
+++ b/drivers/target/tcm_fc/tfc_conf.c
@@ -327,7 +327,6 @@ static struct se_portal_group *ft_add_tpg(
 	tpg->index = index;
 	tpg->lport_acl = lacl;
 	INIT_LIST_HEAD(&tpg->lun_list);
-	transport_init_queue_obj(&tpg->qobj);
 
 	ret = core_tpg_register(&ft_configfs->tf_ops, wwn, &tpg->se_tpg,
 				tpg, TRANSPORT_TPG_TYPE_NORMAL);
@@ -336,8 +335,8 @@ static struct se_portal_group *ft_add_tpg(
 		return NULL;
 	}
 
-	tpg->thread = kthread_run(ft_thread, tpg, "ft_tpg%lu", index);
-	if (IS_ERR(tpg->thread)) {
+	tpg->workqueue = alloc_workqueue("tcm_fc", 0, 1);
+	if (!tpg->workqueue) {
 		kfree(tpg);
 		return NULL;
 	}
@@ -356,7 +355,7 @@ static void ft_del_tpg(struct se_portal_group *se_tpg)
 	pr_debug("del tpg %s\n",
 		    config_item_name(&tpg->se_tpg.tpg_group.cg_item));
 
-	kthread_stop(tpg->thread);
+	destroy_workqueue(tpg->workqueue);
 
 	/* Wait for sessions to be freed thru RCU, for BUG_ON below */
 	synchronize_rcu();
-- 
1.5.6.5

[PATCH 4/5] target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered Extended

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds target_parse_naa_6h_vendor_specific() to address a bug where the
conversion of PRODUCT SERIAL NUMBER to use hex2bin() in target_emulate_evpd_83()
was not doing proper isxdigit() checking.  This conversion of the vpd_unit_serial
configifs attribute is done while generating a EVPD=0x83 NAA IEEE Registered
Extended DESIGNATOR format's 100 bits of unique VENDOR SPECIFIC IDENTIFIER +
VENDOR SPECIFIC IDENTIFIER EXTENSION area.

This patch allows vpd_unit_serial (EVPD=0x80) and the T10 Vendor ID DESIGNATOR
format (EVPD=0x83) to continue to use free-form variable length ASCII values,
and now skips any non hex characters for fixed length NAA IEEE Registered Extended
DESIGNATOR format (EVPD=0x83) requring the binary conversion.

This was originally reported by Martin after the v3.1-rc1 change to use hex2bin()
in commit 11650b859681e03fdbf26277fcfc5f1f62186703 where the use of non hex
characters in vpd_unit_serial generated different values than the original
v3.0 internal hex -> binary code.  This v3.1 change caused a problem with
filesystems who write a NAA DESIGNATOR onto it's ondisk metadata, and this patch
will (again) change existing values to ensure that non hex characters are not
included in the fixed length NAA DESIGNATOR.

Note this patch still expects vpd_unit_serial to be set via existing userspace
methods of uuid generation, and does not do strict formatting via configfs input.

The original bug report and thread can be found here:

NAA breakage
http://www.spinics.net/lists/target-devel/msg00477.html

The v3.1-rc1 formatting of EVPD=0x83 w/o this patch:

VPD INQUIRY: Device Identification page
  Designation descriptor number 1, descriptor length: 20
    designator_type: NAA,  code_set: Binary
    associated with the addressed logical unit
      NAA 6, IEEE Company_id: 0x1405
      Vendor Specific Identifier: 0xffde35ebf
      Vendor Specific Identifier Extension: 0x3092f498ffa820f9
      [0x6001405ffde35ebf3092f498ffa820f9]
  Designation descriptor number 2, descriptor length: 56
    designator_type: T10 vendor identification,  code_set: ASCII
    associated with the addressed logical unit
      vendor id: LIO-ORG
      vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1

The v3.1-final formatting of EVPD=0x83 w/ this patch:

VPD INQUIRY: Device Identification page
  Designation descriptor number 1, descriptor length: 20
    designator_type: NAA,  code_set: Binary
    associated with the addressed logical unit
      NAA 6, IEEE Company_id: 0x1405
      Vendor Specific Identifier: 0xffde35ec3
      Vendor Specific Identifier Extension: 0x924980a82091763
      [0x6001405ffde35ec30924980a82091763]
  Designation descriptor number 2, descriptor length: 56
    designator_type: T10 vendor identification,  code_set: ASCII
    associated with the addressed logical unit
      vendor id: LIO-ORG
      vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1

Reported-by: Martin Svec <martin.svec@zoner.cz>
Cc: Martin Svec <martin.svec@zoner.cz>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: James Bottomley <jbottomley@parallels.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_cdb.c |   35 +++++++++++++++++++++++++++++++++--
 1 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_cdb.c b/drivers/target/target_core_cdb.c
index 89ae923..08bda0c 100644
--- a/drivers/target/target_core_cdb.c
+++ b/drivers/target/target_core_cdb.c
@@ -24,6 +24,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/ctype.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi.h>
 
@@ -154,6 +155,37 @@ target_emulate_evpd_80(struct se_cmd *cmd, unsigned char *buf)
 	return 0;
 }
 
+static void
+target_parse_naa_6h_vendor_specific(struct se_device *dev, unsigned char *buf_off)
+{
+	unsigned char *p = &dev->se_sub_dev->t10_wwn.unit_serial[0];
+	unsigned char *buf = buf_off;
+	int cnt = 0, next = 1;
+	/*
+	 * Generate up to 36 bits of VENDOR SPECIFIC IDENTIFIER starting on
+	 * byte 3 bit 4 for NAA IEEE Registered Extended DESIGNATOR field
+	 * format, followed by 64 bits of VENDOR SPECIFIC IDENTIFIER EXTENSION
+	 * to complete the payload.  These are based from VPD=0x80 PRODUCT SERIAL
+	 * NUMBER set via vpd_unit_serial in target_core_configfs.c  to to ensure
+	 * per device uniqeness.
+	 */
+	while (p != NULL) {
+		if (cnt >= 13)
+			break;
+		if (!isxdigit(*p)) {
+			p++;
+			continue;
+		}
+		if (next != 0) {
+			buf[cnt++] |= hex_to_bin(*p++);
+			next = 0;
+		} else {
+			buf[cnt] |= hex_to_bin(*p++) << 4;
+			next = 1;
+		}
+	}
+}
+
 /*
  * Device identification VPD, for a complete list of
  * DESIGNATOR TYPEs see spc4r17 Table 459.
@@ -219,8 +251,7 @@ target_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
 	 * VENDOR_SPECIFIC_IDENTIFIER and
 	 * VENDOR_SPECIFIC_IDENTIFIER_EXTENTION
 	 */
-	buf[off++] |= hex_to_bin(dev->se_sub_dev->t10_wwn.unit_serial[0]);
-	hex2bin(&buf[off], &dev->se_sub_dev->t10_wwn.unit_serial[1], 12);
+	target_parse_naa_6h_vendor_specific(dev, &buf[off]);
 
 	len = 20;
 	off = (len + 4);
-- 
1.5.6.5

[PATCH 5/5] iscsi-target: Disable markers + remove dangerous local scope array usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch makes iscsi-target explictly disable OFMarker=Yes and IFMarker=yes
parameter key usage during iscsi login by setting IFMarkInt_Reject and
OFMarkInt_Reject values in iscsi_enforce_integrity_rules() to effectively
disable iscsi marker usage.  With this patch, an initiator proposer asking
to enable either marker parameter keys will be issued a 'No' response, and
the target sets OFMarkInt + IFMarkInt parameter key response to 'Irrelevant'.

With markers disabled during iscsi login, this patch removes the problematic
on-stack local-scope array for marker intervals in iscsit_do_rx_data() +
iscsit_do_tx_data(), and other related marker code in iscsi_target_util.c.
This fixes a potentional stack smashing scenario with small range markers
enabled and a large MRDSL as reported by DanC here:

[bug report] target: stack can be smashed
http://www.spinics.net/lists/target-devel/msg00453.html

Reported-by: Dan Carpenter <error27@gmail.com>
Cc: Dan Carpenter <error27@gmail.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_parameters.c |    2 +-
 drivers/target/iscsi/iscsi_target_util.c       |  248 +-----------------------
 2 files changed, 7 insertions(+), 243 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 497b2e7..5b77316 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -1430,7 +1430,7 @@ static int iscsi_enforce_integrity_rules(
 	u8 DataSequenceInOrder = 0;
 	u8 ErrorRecoveryLevel = 0, SessionType = 0;
 	u8 IFMarker = 0, OFMarker = 0;
-	u8 IFMarkInt_Reject = 0, OFMarkInt_Reject = 0;
+	u8 IFMarkInt_Reject = 1, OFMarkInt_Reject = 1;
 	u32 FirstBurstLength = 0, MaxBurstLength = 0;
 	struct iscsi_param *param = NULL;
 
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index a0d23bc..1d1b4fe 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -875,40 +875,6 @@ void iscsit_inc_session_usage_count(struct iscsi_session *sess)
 }
 
 /*
- *	Used before iscsi_do[rx,tx]_data() to determine iov and [rx,tx]_marker
- *	array counts needed for sync and steering.
- */
-static int iscsit_determine_sync_and_steering_counts(
-	struct iscsi_conn *conn,
-	struct iscsi_data_count *count)
-{
-	u32 length = count->data_length;
-	u32 marker, markint;
-
-	count->sync_and_steering = 1;
-
-	marker = (count->type == ISCSI_RX_DATA) ?
-			conn->of_marker : conn->if_marker;
-	markint = (count->type == ISCSI_RX_DATA) ?
-			(conn->conn_ops->OFMarkInt * 4) :
-			(conn->conn_ops->IFMarkInt * 4);
-	count->ss_iov_count = count->iov_count;
-
-	while (length > 0) {
-		if (length >= marker) {
-			count->ss_iov_count += 3;
-			count->ss_marker_count += 2;
-
-			length -= marker;
-			marker = markint;
-		} else
-			length = 0;
-	}
-
-	return 0;
-}
-
-/*
  *	Setup conn->if_marker and conn->of_marker values based upon
  *	the initial marker-less interval. (see iSCSI v19 A.2)
  */
@@ -1431,8 +1397,7 @@ static int iscsit_do_rx_data(
 	struct iscsi_data_count *count)
 {
 	int data = count->data_length, rx_loop = 0, total_rx = 0, iov_len;
-	u32 rx_marker_val[count->ss_marker_count], rx_marker_iov = 0;
-	struct kvec iov[count->ss_iov_count], *iov_p;
+	struct kvec *iov_p;
 	struct msghdr msg;
 
 	if (!conn || !conn->sock || !conn->conn_ops)
@@ -1440,93 +1405,8 @@ static int iscsit_do_rx_data(
 
 	memset(&msg, 0, sizeof(struct msghdr));
 
-	if (count->sync_and_steering) {
-		int size = 0;
-		u32 i, orig_iov_count = 0;
-		u32 orig_iov_len = 0, orig_iov_loc = 0;
-		u32 iov_count = 0, per_iov_bytes = 0;
-		u32 *rx_marker, old_rx_marker = 0;
-		struct kvec *iov_record;
-
-		memset(&rx_marker_val, 0,
-				count->ss_marker_count * sizeof(u32));
-		memset(&iov, 0, count->ss_iov_count * sizeof(struct kvec));
-
-		iov_record = count->iov;
-		orig_iov_count = count->iov_count;
-		rx_marker = &conn->of_marker;
-
-		i = 0;
-		size = data;
-		orig_iov_len = iov_record[orig_iov_loc].iov_len;
-		while (size > 0) {
-			pr_debug("rx_data: #1 orig_iov_len %u,"
-			" orig_iov_loc %u\n", orig_iov_len, orig_iov_loc);
-			pr_debug("rx_data: #2 rx_marker %u, size"
-				" %u\n", *rx_marker, size);
-
-			if (orig_iov_len >= *rx_marker) {
-				iov[iov_count].iov_len = *rx_marker;
-				iov[iov_count++].iov_base =
-					(iov_record[orig_iov_loc].iov_base +
-						per_iov_bytes);
-
-				iov[iov_count].iov_len = (MARKER_SIZE / 2);
-				iov[iov_count++].iov_base =
-					&rx_marker_val[rx_marker_iov++];
-				iov[iov_count].iov_len = (MARKER_SIZE / 2);
-				iov[iov_count++].iov_base =
-					&rx_marker_val[rx_marker_iov++];
-				old_rx_marker = *rx_marker;
-
-				/*
-				 * OFMarkInt is in 32-bit words.
-				 */
-				*rx_marker = (conn->conn_ops->OFMarkInt * 4);
-				size -= old_rx_marker;
-				orig_iov_len -= old_rx_marker;
-				per_iov_bytes += old_rx_marker;
-
-				pr_debug("rx_data: #3 new_rx_marker"
-					" %u, size %u\n", *rx_marker, size);
-			} else {
-				iov[iov_count].iov_len = orig_iov_len;
-				iov[iov_count++].iov_base =
-					(iov_record[orig_iov_loc].iov_base +
-						per_iov_bytes);
-
-				per_iov_bytes = 0;
-				*rx_marker -= orig_iov_len;
-				size -= orig_iov_len;
-
-				if (size)
-					orig_iov_len =
-					iov_record[++orig_iov_loc].iov_len;
-
-				pr_debug("rx_data: #4 new_rx_marker"
-					" %u, size %u\n", *rx_marker, size);
-			}
-		}
-		data += (rx_marker_iov * (MARKER_SIZE / 2));
-
-		iov_p	= &iov[0];
-		iov_len	= iov_count;
-
-		if (iov_count > count->ss_iov_count) {
-			pr_err("iov_count: %d, count->ss_iov_count:"
-				" %d\n", iov_count, count->ss_iov_count);
-			return -1;
-		}
-		if (rx_marker_iov > count->ss_marker_count) {
-			pr_err("rx_marker_iov: %d, count->ss_marker"
-				"_count: %d\n", rx_marker_iov,
-				count->ss_marker_count);
-			return -1;
-		}
-	} else {
-		iov_p = count->iov;
-		iov_len	= count->iov_count;
-	}
+	iov_p = count->iov;
+	iov_len	= count->iov_count;
 
 	while (total_rx < data) {
 		rx_loop = kernel_recvmsg(conn->sock, &msg, iov_p, iov_len,
@@ -1541,16 +1421,6 @@ static int iscsit_do_rx_data(
 				rx_loop, total_rx, data);
 	}
 
-	if (count->sync_and_steering) {
-		int j;
-		for (j = 0; j < rx_marker_iov; j++) {
-			pr_debug("rx_data: #5 j: %d, offset: %d\n",
-				j, rx_marker_val[j]);
-			conn->of_marker_offset = rx_marker_val[j];
-		}
-		total_rx -= (rx_marker_iov * (MARKER_SIZE / 2));
-	}
-
 	return total_rx;
 }
 
@@ -1559,8 +1429,7 @@ static int iscsit_do_tx_data(
 	struct iscsi_data_count *count)
 {
 	int data = count->data_length, total_tx = 0, tx_loop = 0, iov_len;
-	u32 tx_marker_val[count->ss_marker_count], tx_marker_iov = 0;
-	struct kvec iov[count->ss_iov_count], *iov_p;
+	struct kvec *iov_p;
 	struct msghdr msg;
 
 	if (!conn || !conn->sock || !conn->conn_ops)
@@ -1573,98 +1442,8 @@ static int iscsit_do_tx_data(
 
 	memset(&msg, 0, sizeof(struct msghdr));
 
-	if (count->sync_and_steering) {
-		int size = 0;
-		u32 i, orig_iov_count = 0;
-		u32 orig_iov_len = 0, orig_iov_loc = 0;
-		u32 iov_count = 0, per_iov_bytes = 0;
-		u32 *tx_marker, old_tx_marker = 0;
-		struct kvec *iov_record;
-
-		memset(&tx_marker_val, 0,
-			count->ss_marker_count * sizeof(u32));
-		memset(&iov, 0, count->ss_iov_count * sizeof(struct kvec));
-
-		iov_record = count->iov;
-		orig_iov_count = count->iov_count;
-		tx_marker = &conn->if_marker;
-
-		i = 0;
-		size = data;
-		orig_iov_len = iov_record[orig_iov_loc].iov_len;
-		while (size > 0) {
-			pr_debug("tx_data: #1 orig_iov_len %u,"
-			" orig_iov_loc %u\n", orig_iov_len, orig_iov_loc);
-			pr_debug("tx_data: #2 tx_marker %u, size"
-				" %u\n", *tx_marker, size);
-
-			if (orig_iov_len >= *tx_marker) {
-				iov[iov_count].iov_len = *tx_marker;
-				iov[iov_count++].iov_base =
-					(iov_record[orig_iov_loc].iov_base +
-						per_iov_bytes);
-
-				tx_marker_val[tx_marker_iov] =
-						(size - *tx_marker);
-				iov[iov_count].iov_len = (MARKER_SIZE / 2);
-				iov[iov_count++].iov_base =
-					&tx_marker_val[tx_marker_iov++];
-				iov[iov_count].iov_len = (MARKER_SIZE / 2);
-				iov[iov_count++].iov_base =
-					&tx_marker_val[tx_marker_iov++];
-				old_tx_marker = *tx_marker;
-
-				/*
-				 * IFMarkInt is in 32-bit words.
-				 */
-				*tx_marker = (conn->conn_ops->IFMarkInt * 4);
-				size -= old_tx_marker;
-				orig_iov_len -= old_tx_marker;
-				per_iov_bytes += old_tx_marker;
-
-				pr_debug("tx_data: #3 new_tx_marker"
-					" %u, size %u\n", *tx_marker, size);
-				pr_debug("tx_data: #4 offset %u\n",
-					tx_marker_val[tx_marker_iov-1]);
-			} else {
-				iov[iov_count].iov_len = orig_iov_len;
-				iov[iov_count++].iov_base
-					= (iov_record[orig_iov_loc].iov_base +
-						per_iov_bytes);
-
-				per_iov_bytes = 0;
-				*tx_marker -= orig_iov_len;
-				size -= orig_iov_len;
-
-				if (size)
-					orig_iov_len =
-					iov_record[++orig_iov_loc].iov_len;
-
-				pr_debug("tx_data: #5 new_tx_marker"
-					" %u, size %u\n", *tx_marker, size);
-			}
-		}
-
-		data += (tx_marker_iov * (MARKER_SIZE / 2));
-
-		iov_p = &iov[0];
-		iov_len = iov_count;
-
-		if (iov_count > count->ss_iov_count) {
-			pr_err("iov_count: %d, count->ss_iov_count:"
-				" %d\n", iov_count, count->ss_iov_count);
-			return -1;
-		}
-		if (tx_marker_iov > count->ss_marker_count) {
-			pr_err("tx_marker_iov: %d, count->ss_marker"
-				"_count: %d\n", tx_marker_iov,
-				count->ss_marker_count);
-			return -1;
-		}
-	} else {
-		iov_p = count->iov;
-		iov_len = count->iov_count;
-	}
+	iov_p = count->iov;
+	iov_len = count->iov_count;
 
 	while (total_tx < data) {
 		tx_loop = kernel_sendmsg(conn->sock, &msg, iov_p, iov_len,
@@ -1679,9 +1458,6 @@ static int iscsit_do_tx_data(
 					tx_loop, total_tx, data);
 	}
 
-	if (count->sync_and_steering)
-		total_tx -= (tx_marker_iov * (MARKER_SIZE / 2));
-
 	return total_tx;
 }
 
@@ -1702,12 +1478,6 @@ int rx_data(
 	c.data_length = data;
 	c.type = ISCSI_RX_DATA;
 
-	if (conn->conn_ops->OFMarker &&
-	   (conn->conn_state >= TARG_CONN_STATE_LOGGED_IN)) {
-		if (iscsit_determine_sync_and_steering_counts(conn, &c) < 0)
-			return -1;
-	}
-
 	return iscsit_do_rx_data(conn, &c);
 }
 
@@ -1728,12 +1498,6 @@ int tx_data(
 	c.data_length = data;
 	c.type = ISCSI_TX_DATA;
 
-	if (conn->conn_ops->IFMarker &&
-	   (conn->conn_state >= TARG_CONN_STATE_LOGGED_IN)) {
-		if (iscsit_determine_sync_and_steering_counts(conn, &c) < 0)
-			return -1;
-	}
-
 	return iscsit_do_tx_data(conn, &c);
 }
 
-- 
1.5.6.5

Re: [PATCH 4/5] target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered Extended

[Martin Svec]

Tested, looks fine to me. (For next == 0, maybe use assignment instead 
of bitwise OR in "buf[cnt] |= hex_to_bin(*p++) << 4;"?)

Martin

Dne 16.9.2011 12:38, Nicholas A. Bellinger napsal(a):
> From: Nicholas Bellinger<nab@linux-iscsi.org>
>
> This patch adds target_parse_naa_6h_vendor_specific() to address a bug where the
> conversion of PRODUCT SERIAL NUMBER to use hex2bin() in target_emulate_evpd_83()
> was not doing proper isxdigit() checking.  This conversion of the vpd_unit_serial
> configifs attribute is done while generating a EVPD=0x83 NAA IEEE Registered
> Extended DESIGNATOR format's 100 bits of unique VENDOR SPECIFIC IDENTIFIER +
> VENDOR SPECIFIC IDENTIFIER EXTENSION area.
>
> This patch allows vpd_unit_serial (EVPD=0x80) and the T10 Vendor ID DESIGNATOR
> format (EVPD=0x83) to continue to use free-form variable length ASCII values,
> and now skips any non hex characters for fixed length NAA IEEE Registered Extended
> DESIGNATOR format (EVPD=0x83) requring the binary conversion.
>
> This was originally reported by Martin after the v3.1-rc1 change to use hex2bin()
> in commit 11650b859681e03fdbf26277fcfc5f1f62186703 where the use of non hex
> characters in vpd_unit_serial generated different values than the original
> v3.0 internal hex ->  binary code.  This v3.1 change caused a problem with
> filesystems who write a NAA DESIGNATOR onto it's ondisk metadata, and this patch
> will (again) change existing values to ensure that non hex characters are not
> included in the fixed length NAA DESIGNATOR.
>
> Note this patch still expects vpd_unit_serial to be set via existing userspace
> methods of uuid generation, and does not do strict formatting via configfs input.
>
> The original bug report and thread can be found here:
>
> NAA breakage
> http://www.spinics.net/lists/target-devel/msg00477.html
>
> The v3.1-rc1 formatting of EVPD=0x83 w/o this patch:
>
> VPD INQUIRY: Device Identification page
>    Designation descriptor number 1, descriptor length: 20
>      designator_type: NAA,  code_set: Binary
>      associated with the addressed logical unit
>        NAA 6, IEEE Company_id: 0x1405
>        Vendor Specific Identifier: 0xffde35ebf
>        Vendor Specific Identifier Extension: 0x3092f498ffa820f9
>        [0x6001405ffde35ebf3092f498ffa820f9]
>    Designation descriptor number 2, descriptor length: 56
>      designator_type: T10 vendor identification,  code_set: ASCII
>      associated with the addressed logical unit
>        vendor id: LIO-ORG
>        vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
>
> The v3.1-final formatting of EVPD=0x83 w/ this patch:
>
> VPD INQUIRY: Device Identification page
>    Designation descriptor number 1, descriptor length: 20
>      designator_type: NAA,  code_set: Binary
>      associated with the addressed logical unit
>        NAA 6, IEEE Company_id: 0x1405
>        Vendor Specific Identifier: 0xffde35ec3
>        Vendor Specific Identifier Extension: 0x924980a82091763
>        [0x6001405ffde35ec30924980a82091763]
>    Designation descriptor number 2, descriptor length: 56
>      designator_type: T10 vendor identification,  code_set: ASCII
>      associated with the addressed logical unit
>        vendor id: LIO-ORG
>        vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
>
> Reported-by: Martin Svec<martin.svec@zoner.cz>
> Cc: Martin Svec<martin.svec@zoner.cz>
> Cc: Douglas Gilbert<dgilbert@interlog.com>
> Cc: James Bottomley<jbottomley@parallels.com>
> Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_cdb.c |   35 +++++++++++++++++++++++++++++++++--
>   1 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_cdb.c b/drivers/target/target_core_cdb.c
> index 89ae923..08bda0c 100644
> --- a/drivers/target/target_core_cdb.c
> +++ b/drivers/target/target_core_cdb.c
> @@ -24,6 +24,7 @@
>    */
>
>   #include<linux/kernel.h>
> +#include<linux/ctype.h>
>   #include<asm/unaligned.h>
>   #include<scsi/scsi.h>
>
> @@ -154,6 +155,37 @@ target_emulate_evpd_80(struct se_cmd *cmd, unsigned char *buf)
>   	return 0;
>   }
>
> +static void
> +target_parse_naa_6h_vendor_specific(struct se_device *dev, unsigned char *buf_off)
> +{
> +	unsigned char *p =&dev->se_sub_dev->t10_wwn.unit_serial[0];
> +	unsigned char *buf = buf_off;
> +	int cnt = 0, next = 1;
> +	/*
> +	 * Generate up to 36 bits of VENDOR SPECIFIC IDENTIFIER starting on
> +	 * byte 3 bit 4 for NAA IEEE Registered Extended DESIGNATOR field
> +	 * format, followed by 64 bits of VENDOR SPECIFIC IDENTIFIER EXTENSION
> +	 * to complete the payload.  These are based from VPD=0x80 PRODUCT SERIAL
> +	 * NUMBER set via vpd_unit_serial in target_core_configfs.c  to to ensure
> +	 * per device uniqeness.
> +	 */
> +	while (p != NULL) {
> +		if (cnt>= 13)
> +			break;
> +		if (!isxdigit(*p)) {
> +			p++;
> +			continue;
> +		}
> +		if (next != 0) {
> +			buf[cnt++] |= hex_to_bin(*p++);
> +			next = 0;
> +		} else {
> +			buf[cnt] |= hex_to_bin(*p++)<<  4;
> +			next = 1;
> +		}
> +	}
> +}
> +
>   /*
>    * Device identification VPD, for a complete list of
>    * DESIGNATOR TYPEs see spc4r17 Table 459.
> @@ -219,8 +251,7 @@ target_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
>   	 * VENDOR_SPECIFIC_IDENTIFIER and
>   	 * VENDOR_SPECIFIC_IDENTIFIER_EXTENTION
>   	 */
> -	buf[off++] |= hex_to_bin(dev->se_sub_dev->t10_wwn.unit_serial[0]);
> -	hex2bin(&buf[off],&dev->se_sub_dev->t10_wwn.unit_serial[1], 12);
> +	target_parse_naa_6h_vendor_specific(dev,&buf[off]);
>
>   	len = 20;
>   	off = (len + 4);

Re: [PATCH 4/5] target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered Extended

[Douglas Gilbert]

A documentation note: it should be "VPD=0x83" not "EVPD=0x83"
in the subject line and elsewhere.

The "E" stands for "enable" and EVPD refers to a bit field in a
INQUIRY cdb. When it is set the response should be a VPD page
corresponding to the "page code" field in the cdb.

I made the same mistake in my tools but I hope that has all
been cleaned up now.

Doug Gilbert


On 11-09-16 09:59 AM, Martin Svec wrote:
> Tested, looks fine to me. (For next == 0, maybe use assignment instead of
> bitwise OR in "buf[cnt] |= hex_to_bin(*p++) << 4;"?)
>
> Martin
>
> Dne 16.9.2011 12:38, Nicholas A. Bellinger napsal(a):
>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>
>> This patch adds target_parse_naa_6h_vendor_specific() to address a bug where the
>> conversion of PRODUCT SERIAL NUMBER to use hex2bin() in target_emulate_evpd_83()
>> was not doing proper isxdigit() checking. This conversion of the vpd_unit_serial
>> configifs attribute is done while generating a EVPD=0x83 NAA IEEE Registered
>> Extended DESIGNATOR format's 100 bits of unique VENDOR SPECIFIC IDENTIFIER +
>> VENDOR SPECIFIC IDENTIFIER EXTENSION area.
>>
>> This patch allows vpd_unit_serial (EVPD=0x80) and the T10 Vendor ID DESIGNATOR
>> format (EVPD=0x83) to continue to use free-form variable length ASCII values,
>> and now skips any non hex characters for fixed length NAA IEEE Registered
>> Extended
>> DESIGNATOR format (EVPD=0x83) requring the binary conversion.
>>
>> This was originally reported by Martin after the v3.1-rc1 change to use hex2bin()
>> in commit 11650b859681e03fdbf26277fcfc5f1f62186703 where the use of non hex
>> characters in vpd_unit_serial generated different values than the original
>> v3.0 internal hex -> binary code. This v3.1 change caused a problem with
>> filesystems who write a NAA DESIGNATOR onto it's ondisk metadata, and this patch
>> will (again) change existing values to ensure that non hex characters are not
>> included in the fixed length NAA DESIGNATOR.
>>
>> Note this patch still expects vpd_unit_serial to be set via existing userspace
>> methods of uuid generation, and does not do strict formatting via configfs input.
>>
>> The original bug report and thread can be found here:
>>
>> NAA breakage
>> http://www.spinics.net/lists/target-devel/msg00477.html
>>
>> The v3.1-rc1 formatting of EVPD=0x83 w/o this patch:
>>
>> VPD INQUIRY: Device Identification page
>> Designation descriptor number 1, descriptor length: 20
>> designator_type: NAA, code_set: Binary
>> associated with the addressed logical unit
>> NAA 6, IEEE Company_id: 0x1405
>> Vendor Specific Identifier: 0xffde35ebf
>> Vendor Specific Identifier Extension: 0x3092f498ffa820f9
>> [0x6001405ffde35ebf3092f498ffa820f9]
>> Designation descriptor number 2, descriptor length: 56
>> designator_type: T10 vendor identification, code_set: ASCII
>> associated with the addressed logical unit
>> vendor id: LIO-ORG
>> vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
>>
>> The v3.1-final formatting of EVPD=0x83 w/ this patch:
>>
>> VPD INQUIRY: Device Identification page
>> Designation descriptor number 1, descriptor length: 20
>> designator_type: NAA, code_set: Binary
>> associated with the addressed logical unit
>> NAA 6, IEEE Company_id: 0x1405
>> Vendor Specific Identifier: 0xffde35ec3
>> Vendor Specific Identifier Extension: 0x924980a82091763
>> [0x6001405ffde35ec30924980a82091763]
>> Designation descriptor number 2, descriptor length: 56
>> designator_type: T10 vendor identification, code_set: ASCII
>> associated with the addressed logical unit
>> vendor id: LIO-ORG
>> vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
>>
>> Reported-by: Martin Svec<martin.svec@zoner.cz>
>> Cc: Martin Svec<martin.svec@zoner.cz>
>> Cc: Douglas Gilbert<dgilbert@interlog.com>
>> Cc: James Bottomley<jbottomley@parallels.com>
>> Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org>
>> ---
>> drivers/target/target_core_cdb.c | 35 +++++++++++++++++++++++++++++++++--
>> 1 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/target/target_core_cdb.c b/drivers/target/target_core_cdb.c
>> index 89ae923..08bda0c 100644
>> --- a/drivers/target/target_core_cdb.c
>> +++ b/drivers/target/target_core_cdb.c
>> @@ -24,6 +24,7 @@
>> */
>>
>> #include<linux/kernel.h>
>> +#include<linux/ctype.h>
>> #include<asm/unaligned.h>
>> #include<scsi/scsi.h>
>>
>> @@ -154,6 +155,37 @@ target_emulate_evpd_80(struct se_cmd *cmd, unsigned char
>> *buf)
>> return 0;
>> }
>>
>> +static void
>> +target_parse_naa_6h_vendor_specific(struct se_device *dev, unsigned char
>> *buf_off)
>> +{
>> + unsigned char *p =&dev->se_sub_dev->t10_wwn.unit_serial[0];
>> + unsigned char *buf = buf_off;
>> + int cnt = 0, next = 1;
>> + /*
>> + * Generate up to 36 bits of VENDOR SPECIFIC IDENTIFIER starting on
>> + * byte 3 bit 4 for NAA IEEE Registered Extended DESIGNATOR field
>> + * format, followed by 64 bits of VENDOR SPECIFIC IDENTIFIER EXTENSION
>> + * to complete the payload. These are based from VPD=0x80 PRODUCT SERIAL
>> + * NUMBER set via vpd_unit_serial in target_core_configfs.c to to ensure
>> + * per device uniqeness.
>> + */
>> + while (p != NULL) {
>> + if (cnt>= 13)
>> + break;
>> + if (!isxdigit(*p)) {
>> + p++;
>> + continue;
>> + }
>> + if (next != 0) {
>> + buf[cnt++] |= hex_to_bin(*p++);
>> + next = 0;
>> + } else {
>> + buf[cnt] |= hex_to_bin(*p++)<< 4;
>> + next = 1;
>> + }
>> + }
>> +}
>> +
>> /*
>> * Device identification VPD, for a complete list of
>> * DESIGNATOR TYPEs see spc4r17 Table 459.
>> @@ -219,8 +251,7 @@ target_emulate_evpd_83(struct se_cmd *cmd, unsigned char
>> *buf)
>> * VENDOR_SPECIFIC_IDENTIFIER and
>> * VENDOR_SPECIFIC_IDENTIFIER_EXTENTION
>> */
>> - buf[off++] |= hex_to_bin(dev->se_sub_dev->t10_wwn.unit_serial[0]);
>> - hex2bin(&buf[off],&dev->se_sub_dev->t10_wwn.unit_serial[1], 12);
>> + target_parse_naa_6h_vendor_specific(dev,&buf[off]);
>>
>> len = 20;
>> off = (len + 4);
>
>

Re: [PATCH 4/5] target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered Extended

[Nicholas A. Bellinger]

On Fri, 2011-09-16 at 15:59 +0200, Martin Svec wrote:
> Tested, looks fine to me. (For next == 0, maybe use assignment instead 
> of bitwise OR in "buf[cnt] |= hex_to_bin(*p++) << 4;"?)
> 

Fair enough.  Making this minor change now.

Thanks for testing Martin!

--nab

> Martin
> 
> Dne 16.9.2011 12:38, Nicholas A. Bellinger napsal(a):
> > From: Nicholas Bellinger<nab@linux-iscsi.org>
> >
> > This patch adds target_parse_naa_6h_vendor_specific() to address a bug where the
> > conversion of PRODUCT SERIAL NUMBER to use hex2bin() in target_emulate_evpd_83()
> > was not doing proper isxdigit() checking.  This conversion of the vpd_unit_serial
> > configifs attribute is done while generating a EVPD=0x83 NAA IEEE Registered
> > Extended DESIGNATOR format's 100 bits of unique VENDOR SPECIFIC IDENTIFIER +
> > VENDOR SPECIFIC IDENTIFIER EXTENSION area.
> >
> > This patch allows vpd_unit_serial (EVPD=0x80) and the T10 Vendor ID DESIGNATOR
> > format (EVPD=0x83) to continue to use free-form variable length ASCII values,
> > and now skips any non hex characters for fixed length NAA IEEE Registered Extended
> > DESIGNATOR format (EVPD=0x83) requring the binary conversion.
> >
> > This was originally reported by Martin after the v3.1-rc1 change to use hex2bin()
> > in commit 11650b859681e03fdbf26277fcfc5f1f62186703 where the use of non hex
> > characters in vpd_unit_serial generated different values than the original
> > v3.0 internal hex ->  binary code.  This v3.1 change caused a problem with
> > filesystems who write a NAA DESIGNATOR onto it's ondisk metadata, and this patch
> > will (again) change existing values to ensure that non hex characters are not
> > included in the fixed length NAA DESIGNATOR.
> >
> > Note this patch still expects vpd_unit_serial to be set via existing userspace
> > methods of uuid generation, and does not do strict formatting via configfs input.
> >
> > The original bug report and thread can be found here:
> >
> > NAA breakage
> > http://www.spinics.net/lists/target-devel/msg00477.html
> >
> > The v3.1-rc1 formatting of EVPD=0x83 w/o this patch:
> >
> > VPD INQUIRY: Device Identification page
> >    Designation descriptor number 1, descriptor length: 20
> >      designator_type: NAA,  code_set: Binary
> >      associated with the addressed logical unit
> >        NAA 6, IEEE Company_id: 0x1405
> >        Vendor Specific Identifier: 0xffde35ebf
> >        Vendor Specific Identifier Extension: 0x3092f498ffa820f9
> >        [0x6001405ffde35ebf3092f498ffa820f9]
> >    Designation descriptor number 2, descriptor length: 56
> >      designator_type: T10 vendor identification,  code_set: ASCII
> >      associated with the addressed logical unit
> >        vendor id: LIO-ORG
> >        vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
> >
> > The v3.1-final formatting of EVPD=0x83 w/ this patch:
> >
> > VPD INQUIRY: Device Identification page
> >    Designation descriptor number 1, descriptor length: 20
> >      designator_type: NAA,  code_set: Binary
> >      associated with the addressed logical unit
> >        NAA 6, IEEE Company_id: 0x1405
> >        Vendor Specific Identifier: 0xffde35ec3
> >        Vendor Specific Identifier Extension: 0x924980a82091763
> >        [0x6001405ffde35ec30924980a82091763]
> >    Designation descriptor number 2, descriptor length: 56
> >      designator_type: T10 vendor identification,  code_set: ASCII
> >      associated with the addressed logical unit
> >        vendor id: LIO-ORG
> >        vendor specific: IBLOCK:ffde35ec-3092-4980-a820-917636ca54f1
> >
> > Reported-by: Martin Svec<martin.svec@zoner.cz>
> > Cc: Martin Svec<martin.svec@zoner.cz>
> > Cc: Douglas Gilbert<dgilbert@interlog.com>
> > Cc: James Bottomley<jbottomley@parallels.com>
> > Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_cdb.c |   35 +++++++++++++++++++++++++++++++++--
> >   1 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/target/target_core_cdb.c b/drivers/target/target_core_cdb.c
> > index 89ae923..08bda0c 100644
> > --- a/drivers/target/target_core_cdb.c
> > +++ b/drivers/target/target_core_cdb.c
> > @@ -24,6 +24,7 @@
> >    */
> >
> >   #include<linux/kernel.h>
> > +#include<linux/ctype.h>
> >   #include<asm/unaligned.h>
> >   #include<scsi/scsi.h>
> >
> > @@ -154,6 +155,37 @@ target_emulate_evpd_80(struct se_cmd *cmd, unsigned char *buf)
> >   	return 0;
> >   }
> >
> > +static void
> > +target_parse_naa_6h_vendor_specific(struct se_device *dev, unsigned char *buf_off)
> > +{
> > +	unsigned char *p =&dev->se_sub_dev->t10_wwn.unit_serial[0];
> > +	unsigned char *buf = buf_off;
> > +	int cnt = 0, next = 1;
> > +	/*
> > +	 * Generate up to 36 bits of VENDOR SPECIFIC IDENTIFIER starting on
> > +	 * byte 3 bit 4 for NAA IEEE Registered Extended DESIGNATOR field
> > +	 * format, followed by 64 bits of VENDOR SPECIFIC IDENTIFIER EXTENSION
> > +	 * to complete the payload.  These are based from VPD=0x80 PRODUCT SERIAL
> > +	 * NUMBER set via vpd_unit_serial in target_core_configfs.c  to to ensure
> > +	 * per device uniqeness.
> > +	 */
> > +	while (p != NULL) {
> > +		if (cnt>= 13)
> > +			break;
> > +		if (!isxdigit(*p)) {
> > +			p++;
> > +			continue;
> > +		}
> > +		if (next != 0) {
> > +			buf[cnt++] |= hex_to_bin(*p++);
> > +			next = 0;
> > +		} else {
> > +			buf[cnt] |= hex_to_bin(*p++)<<  4;
> > +			next = 1;
> > +		}
> > +	}
> > +}
> > +
> >   /*
> >    * Device identification VPD, for a complete list of
> >    * DESIGNATOR TYPEs see spc4r17 Table 459.
> > @@ -219,8 +251,7 @@ target_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
> >   	 * VENDOR_SPECIFIC_IDENTIFIER and
> >   	 * VENDOR_SPECIFIC_IDENTIFIER_EXTENTION
> >   	 */
> > -	buf[off++] |= hex_to_bin(dev->se_sub_dev->t10_wwn.unit_serial[0]);
> > -	hex2bin(&buf[off],&dev->se_sub_dev->t10_wwn.unit_serial[1], 12);
> > +	target_parse_naa_6h_vendor_specific(dev,&buf[off]);
> >
> >   	len = 20;
> >   	off = (len + 4);
> 

Re: [PATCH 4/5] target: Skip non hex characters for EVPD=0x83 NAA IEEE Registered Extended

[Nicholas A. Bellinger]

On Fri, 2011-09-16 at 10:19 -0400, Douglas Gilbert wrote:
> A documentation note: it should be "VPD=0x83" not "EVPD=0x83"
> in the subject line and elsewhere.
> 
> The "E" stands for "enable" and EVPD refers to a bit field in a
> INQUIRY cdb. When it is set the response should be a VPD page
> corresponding to the "page code" field in the cdb.
> 
> I made the same mistake in my tools but I hope that has all
> been cleaned up now.
> 

Ah yes, fixing this up in the commit and comments now.

Thanks Doug!

--nab

Re: [PATCH 1/5] target: Fix race between multiple invocations of target_qf_do_work()

[Linus Torvalds]

On Fri, Sep 16, 2011 at 3:38 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> When work is scheduled with schedule_work(), the work can end up
> running on multiple CPUs at the same time -- this happens if
> the work is already running on one CPU and schedule_work() is called
> on another CPU.  This leads to list corruption with target_qf_do_work(),
> which is roughly doing:
>
>        spin_lock(...);
>        list_for_each_entry_safe(...) {
>                list_del(...);
>                spin_unlock(...);
>
>                // do stuff
>
>                spin_lock(...);
>        }
>
> With multiple CPUs running this code, one CPU can end up deleting the
> list entry that the other CPU is about to work on.
>
> Fix this by splicing the list entries onto a local list and then
> operating on that in the work function.

Umm. It sounds like what you really want is just a single-threaded workqueue.

Wouldn't it be better to do the alloc_workqueue with WQ_UNBOUND, and a
max limit of a single thread? There's a helper function for it:
alloc_ordered_workqueue().

I dunno. Maybe there's a reason why you actually do want threaded
workqueues, but your description makes it sound like this would be
better resolved by simply using an ordered on.

                      Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] target: Fix race between multiple invocations of target_qf_do_work()

[Christoph Hellwig]

On Sat, Sep 17, 2011 at 12:23:36PM -0700, Linus Torvalds wrote:
> > With multiple CPUs running this code, one CPU can end up deleting the
> > list entry that the other CPU is about to work on.
> >
> > Fix this by splicing the list entries onto a local list and then
> > operating on that in the work function.
> 
> Umm. It sounds like what you really want is just a single-threaded workqueue.
> 
> Wouldn't it be better to do the alloc_workqueue with WQ_UNBOUND, and a
> max limit of a single thread? There's a helper function for it:
> alloc_ordered_workqueue().
> 
> I dunno. Maybe there's a reason why you actually do want threaded
> workqueues, but your description makes it sound like this would be
> better resolved by simply using an ordered on.

The right fix is to restructure the code at an higher level, and queue
up each individual command to the workqueue, thus killing the list
entirely.  But that's not really in scope for 3.1 at this point.