Avoid that enabling a target too quickly triggers a crash

The sysfs attributes of a target can be accessed before scst_tgt.tgt_priv has been set. Avoid that such an access triggers a crash. Symptom: BUG: unable to handle kernel NULL pointer dereference at 000000000000004c IP: [<ffffffffa1345848>] iscsi_is_target_enabled+0x8/0x20 [iscsi_scst] PGD 150a4c067 PUD 13cacb067 PMD 0 Oops: 0000 [#1] SMP CPU 1 Modules linked in: zfs(P) iscsi_scst(O) scst_vdisk(O) scst(O) zcommon(P) znvpair(P) zavl(P) zunicode(P) spl(O) [last unloaded: zfs] Pid: 3974, comm: scst_uid1864 Tainted: P O 3.2.23-zfsiscsi-std-ipv6-64 #3 Supermicro X6DVA/X6DVA RIP: 0010:[<ffffffffa1345848>] [<ffffffffa1345848>] iscsi_is_target_enabled+0x8/0x20 [iscsi_scst] RSP: 0018:ffff88015071ddc0 EFLAGS: 00010282 RAX: 0000000000000000 RBX: ffff880181fdaa00 RCX: ffff88012d41f380 RDX: ffff88015071c010 RSI: 0000000000000000 RDI: ffff880181fdaa00 RBP: ffff88015071ddc0 R08: 0000000000000000 R09: 0000000000000001 R10: 0000000000000001 R11: 0000000000000001 R12: ffffffffa134a608 R13: 0000000000000000 R14: 0000000000000129 R15: ffffffffa134a4c0 FS: 0000000000000000(0000) GS:ffff88019fc80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 000000000000004c CR3: 0000000151927000 CR4: 00000000000006e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process scst_uid1864 (pid: 3974, threadinfo ffff88015071c000, task ffff880194d57500) Stack: ffff88015071de10 ffffffffa12ee89a 00000000fffffffc 01ff88015071de98 ffff88015071de80 ffff8801932a48d8 00000000fffffffc ffff88015071de98 ffff88015071de80 ffff880194d57500 ffff88015071de30 ffffffffa12f582e Call Trace: [<ffffffffa12ee89a>] __scst_is_relative_target_port_id_unique+0x6a/0xc0 [scst] [<ffffffffa12f582e>] gen_relative_target_port_id+0x6e/0xd0 [scst] [<ffffffffa12fdb8a>] scst_tgt_enable_store_work_fn+0x3a/0xe0 [scst] [<ffffffffa12f9397>] sysfs_work_thread_fn+0xc7/0x2e0 [scst] [<ffffffffa12f92d0>] ? scst_tgtt_release+0x20/0x20 [scst] [<ffffffffa12f92d0>] ? scst_tgtt_release+0x20/0x20 [scst] [<ffffffff810b44f6>] kthread+0x96/0xa0 [<ffffffff81b8ed74>] kernel_thread_helper+0x4/0x10 [<ffffffff810b4460>] ? kthread_worker_fn+0x180/0x180 [<ffffffff81b8ed70>] ? gs_change+0xb/0xb Code: 42 c8 74 0f 39 78 48 75 ea c9 c3 0f 1f 84 00 00 00 00 00 31 c0 c9 c3 66 66 66 2e 0f 1f 84 00 00 00 00 00 48 8b 47 48 55 48 89 e5 <0f> b6 40 4c c9 83 e0 01 c3 66 66 66 66 66 66 2e 0f 1f 84 00 00 RIP [<ffffffffa1345848>] iscsi_is_target_enabled+0x8/0x20 [iscsi_scst] RSP <ffff88015071ddc0> CR2: 000000000000004c ---[ end trace 6311d4069703be07 ]--- Reported-by: Etienne Dechamps <e-t172@akegroup.org> Signed-off-by: Bart Van Assche <bvanassche@acm.org> with some modifications git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@4515 d57e44dd-8a1f-0410-8b47-8ef2f437770f
2026-08-17 20:56:24 +00:00 · 2012-09-12 22:13:43 +00:00
parent 14e29d815e
commit 8ce28aaba2
6 changed files with 123 additions and 47 deletions
@@ -564,15 +564,22 @@ int ft_tgt_enable(struct scst_tgt *tgt, bool enable)
 	int ret = 0;

 	mutex_lock(&ft_lport_lock);
+
 	if (enable) {
 		FT_SESS_DBG("enable tgt %s\n", tgt->tgt_name);
 		tport = scst_tgt_get_tgt_priv(tgt);
+		if (tport == NULL) {
+			ret = -E_TGT_PRIV_NOT_YET_SET;
+			goto out_unlock;
+		}
 		tport->enabled = 1;
 		tport->lport->service_params |= FCP_SPPF_TARG_FCN;
 	} else {
 		FT_SESS_DBG("disable tgt %s\n", tgt->tgt_name);
 		ft_tgt_release(tgt);
 	}
+
+out_unlock:
 	mutex_unlock(&ft_lport_lock);
 	return ret;
 }
@@ -581,6 +588,9 @@ bool ft_tgt_enabled(struct scst_tgt *tgt)
 {
 	struct ft_tport *tport;

+	if (tgt == NULL)
+		return false;
+
 	tport = scst_tgt_get_tgt_priv(tgt);
 	return tport->enabled;
 }
@@ -454,19 +454,22 @@ const struct seq_operations iscsi_seq_op = {
 static ssize_t iscsi_tgt_tid_show(struct kobject *kobj,
 	struct kobj_attribute *attr, char *buf)
 {
-	int pos;
+	int res = -E_TGT_PRIV_NOT_YET_SET;
 	struct scst_tgt *scst_tgt;
 	struct iscsi_target *tgt;

 	TRACE_ENTRY();

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct iscsi_target *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;

-	pos = sprintf(buf, "%u\n", tgt->tid);
+	res = sprintf(buf, "%u\n", tgt->tid);

-	TRACE_EXIT_RES(pos);
-	return pos;
+out:
+	TRACE_EXIT_RES(res);
+	return res;
 }

 static struct kobj_attribute iscsi_tgt_attr_tid =
@@ -532,6 +535,11 @@ int iscsi_enable_target(struct scst_tgt *scst_tgt, bool enable)

 	TRACE_ENTRY();

+	if (tgt == NULL) {
+		res = -E_TGT_PRIV_NOT_YET_SET;
+		goto out;
+	}
+
 	if (enable)
 		type = E_ENABLE_TARGET;
 	else
@@ -541,6 +549,7 @@ int iscsi_enable_target(struct scst_tgt *scst_tgt, bool enable)

 	res = iscsi_sysfs_send_event(tgt->tid, type, NULL, NULL, NULL);

+out:
 	TRACE_EXIT_RES(res);
 	return res;
 }
@@ -550,7 +559,10 @@ bool iscsi_is_target_enabled(struct scst_tgt *scst_tgt)
 	struct iscsi_target *tgt =
 		(struct iscsi_target *)scst_tgt_get_tgt_priv(scst_tgt);

-	return tgt->tgt_enabled;
+	if (tgt != NULL)
+		return tgt->tgt_enabled;
+	else
+		return false;
 }

 ssize_t iscsi_sysfs_add_target(const char *target_name, char *params)
@@ -5969,23 +5969,31 @@ out:
 static int q2t_enable_tgt(struct scst_tgt *scst_tgt, bool enable)
 {
 	struct q2t_tgt *tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
-	scsi_qla_host_t *ha = tgt->ha;
-	int res;
+	scsi_qla_host_t *ha;
+	int res = -E_TGT_PRIV_NOT_YET_SET;
+
+	if (tgt == NULL)
+		goto out;
+
+	ha = tgt->ha;

 	if (enable)
 		res = q2t_host_action(ha, ENABLE_TARGET_MODE);
 	else
 		res = q2t_host_action(ha, DISABLE_TARGET_MODE);

+out:
 	return res;
 }

 static bool q2t_is_tgt_enabled(struct scst_tgt *scst_tgt)
 {
 	struct q2t_tgt *tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
-	scsi_qla_host_t *ha = tgt->ha;

-	return qla_tgt_mode_enabled(ha);
+	if (tgt == NULL)
+		return false;
+
+	return qla_tgt_mode_enabled(tgt->ha);
 }

 #if ((LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 28)) || \
@@ -6193,16 +6201,19 @@ static ssize_t q2t_show_expl_conf_enabled(struct kobject *kobj,
 	struct scst_tgt *scst_tgt;
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha;
-	ssize_t size;
+	int res = -E_TGT_PRIV_NOT_YET_SET;

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = tgt->ha;

-	size = scnprintf(buffer, PAGE_SIZE, "%d\n%s", ha->enable_explicit_conf,
+	res = scnprintf(buffer, PAGE_SIZE, "%d\n%s", ha->enable_explicit_conf,
 		ha->enable_explicit_conf ? SCST_SYSFS_KEY_MARK "\n" : "");

-	return size;
+out:
+	return res;
 }

 static ssize_t q2t_store_expl_conf_enabled(struct kobject *kobj,
@@ -6211,10 +6222,13 @@ static ssize_t q2t_store_expl_conf_enabled(struct kobject *kobj,
 	struct scst_tgt *scst_tgt;
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha, *pha;
+	int res = -E_TGT_PRIV_NOT_YET_SET;
 	unsigned long flags;

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = tgt->ha;
 	pha = to_qla_parent(ha);

@@ -6239,7 +6253,10 @@ static ssize_t q2t_store_expl_conf_enabled(struct kobject *kobj,

 	spin_unlock_irqrestore(&pha->hardware_lock, flags);

-	return size;
+	res = size;
+
+out:
+	return res;
 }

 static ssize_t q2t_abort_isp_store(struct kobject *kobj,
@@ -6248,9 +6265,12 @@ static ssize_t q2t_abort_isp_store(struct kobject *kobj,
 	struct scst_tgt *scst_tgt;
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha;
+	int res = -E_TGT_PRIV_NOT_YET_SET;

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = tgt->ha;

 	PRINT_INFO("qla2x00t(%ld): Aborting ISP", ha->instance);
@@ -6258,7 +6278,10 @@ static ssize_t q2t_abort_isp_store(struct kobject *kobj,
 	set_bit(ISP_ABORT_NEEDED, &ha->dpc_flags);
 	qla2x00_wait_for_hba_online(ha);

-	return size;
+	res = size;
+
+out:
+	return res;
 }

 static ssize_t q2t_version_show(struct kobject *kobj,
@@ -6298,12 +6321,14 @@ static ssize_t q2t_node_name_show(struct kobject *kobj,
 	struct scst_tgt *scst_tgt;
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha;
-	ssize_t res;
+	ssize_t res = -E_TGT_PRIV_NOT_YET_SET;
 	char *wwn;
 	uint8_t *node_name;

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = tgt->ha;

 	if (ha->parent == NULL) {
@@ -6335,12 +6360,14 @@ static ssize_t q2t_node_name_store(struct kobject *kobj,
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha;
 	u64 node_name, old_node_name;
-	int res;
+	int res = -E_TGT_PRIV_NOT_YET_SET;

 	TRACE_ENTRY();

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = tgt->ha;

 	sBUG_ON(ha->parent != NULL);
@@ -6387,11 +6414,13 @@ static ssize_t q2t_vp_parent_host_show(struct kobject *kobj,
 	struct scst_tgt *scst_tgt;
 	struct q2t_tgt *tgt;
 	scsi_qla_host_t *ha;
-	ssize_t res;
+	ssize_t res = -E_TGT_PRIV_NOT_YET_SET;
 	char *wwn;

 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
-	tgt = (struct q2t_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	tgt = scst_tgt_get_tgt_priv(scst_tgt);
+	if (!tgt)
+		goto out;
 	ha = to_qla_parent(tgt->ha);

 	res = q2t_get_target_name(ha->port_name, &wwn);
@@ -32,6 +32,10 @@
 #endif
 #include <scsi/scsi.h>

+#ifndef __KERNEL__
+#include <errno.h>
+#endif
+
 /*
 * Version numbers, the same as for the kernel.
 *
@@ -576,4 +580,13 @@ enum scst_tg_sup {
 #define SCST_MIN_REL_TGT_ID			1
 #define SCST_MAX_REL_TGT_ID			65535

+/*
+ * Error code returned by target attribute sysfs methods if invoked after
+ * scst_register_target() finished but before before scst_tgt_set_tgt_priv()
+ * has been invoked.
+ */
+enum {
+	E_TGT_PRIV_NOT_YET_SET = EBUSY
+};
+
 #endif /* __SCST_CONST_H */
@@ -394,6 +394,8 @@ static ssize_t scst_local_scsi_transport_version_show(struct kobject *kobj,
 	if (down_read_trylock(&scst_local_exit_rwsem) == 0)
 		goto out;

+	res = -E_TGT_PRIV_NOT_YET_SET;
+
 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
 	tgt = scst_tgt_get_tgt_priv(scst_tgt);
 	if (!tgt)
@@ -422,6 +424,8 @@ static ssize_t scst_local_scsi_transport_version_store(struct kobject *kobj,
 	if (down_read_trylock(&scst_local_exit_rwsem) == 0)
 		goto out;

+	res = -E_TGT_PRIV_NOT_YET_SET;
+
 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
 	tgt = scst_tgt_get_tgt_priv(scst_tgt);
 	if (!tgt)
@@ -462,6 +466,8 @@ static ssize_t scst_local_phys_transport_version_show(struct kobject *kobj,
 	if (down_read_trylock(&scst_local_exit_rwsem) == 0)
 		goto out;

+	res = -E_TGT_PRIV_NOT_YET_SET;
+
 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
 	tgt = scst_tgt_get_tgt_priv(scst_tgt);
 	if (!tgt)
@@ -488,6 +494,8 @@ static ssize_t scst_local_phys_transport_version_store(struct kobject *kobj,
 	if (down_read_trylock(&scst_local_exit_rwsem) == 0)
 		goto out;

+	res = -E_TGT_PRIV_NOT_YET_SET;
+
 	scst_tgt = container_of(kobj, struct scst_tgt, tgt_kobj);
 	tgt = scst_tgt_get_tgt_priv(scst_tgt);
 	if (!tgt)
@@ -1339,9 +1347,12 @@ static void scst_local_targ_task_mgmt_done(struct scst_mgmt_cmd *mgmt_cmd)

 static uint16_t scst_local_get_scsi_transport_version(struct scst_tgt *scst_tgt)
 {
-	struct scst_local_tgt *tgt;
+	struct scst_local_tgt *tgt = (struct scst_local_tgt *)scst_tgt_get_tgt_priv(scst_tgt);

-	tgt = (struct scst_local_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	/*
+	 * It's OK to not check tgt != NULL here, because new sessions
+	 * can't create before its' set.
+	 */

 	if (tgt->scsi_transport_version == 0)
 		return 0x0BE0; /* SAS */
@@ -1351,9 +1362,12 @@ static uint16_t scst_local_get_scsi_transport_version(struct scst_tgt *scst_tgt)

 static uint16_t scst_local_get_phys_transport_version(struct scst_tgt *scst_tgt)
 {
-	struct scst_local_tgt *tgt;
+	struct scst_local_tgt *tgt = (struct scst_local_tgt *)scst_tgt_get_tgt_priv(scst_tgt);

-	tgt = (struct scst_local_tgt *)scst_tgt_get_tgt_priv(scst_tgt);
+	/*
+	 * It's OK to not check tgt != NULL here, because new sessions
+	 * can't create before its' set.
+	 */

 	return tgt->phys_transport_version;
 }
@@ -2365,12 +2365,10 @@ static struct srpt_tgt *srpt_convert_scst_tgt(struct scst_tgt *scst_tgt)

 	if (one_target_per_port) {
 		sport = scst_tgt_get_tgt_priv(scst_tgt);
-		BUG_ON(!sport);
-		srpt_tgt = &sport->srpt_tgt;
+		srpt_tgt = sport ? &sport->srpt_tgt : NULL;
 	} else {
 		sdev = scst_tgt_get_tgt_priv(scst_tgt);
-		BUG_ON(!sdev);
-		srpt_tgt = &sdev->srpt_tgt;
+		srpt_tgt = sdev ? &sdev->srpt_tgt : NULL;
 	}
 	return srpt_tgt;
 }
@@ -2386,7 +2384,7 @@ static int srpt_enable_target(struct scst_tgt *scst_tgt, bool enable)
 	EXTRACHECKS_WARN_ON_ONCE(irqs_disabled());

 	if (!srpt_tgt)
-		return -ENOENT;
+		return -E_TGT_PRIV_NOT_YET_SET;

 	TRACE_DBG("%s target %s", enable ? "Enabling" : "Disabling",
 		  scst_tgt->tgt_name);
@@ -2407,7 +2405,10 @@ static bool srpt_is_target_enabled(struct scst_tgt *scst_tgt)
 {
 	struct srpt_tgt *srpt_tgt = srpt_convert_scst_tgt(scst_tgt);

-	return srpt_tgt && srpt_tgt->enabled;
+	if (srpt_tgt)
+		return srpt_tgt->enabled;
+	else
+		return false;
 }
 #endif

@@ -3569,14 +3570,7 @@ static int srpt_release(struct scst_tgt *scst_tgt)
 	EXTRACHECKS_WARN_ON_ONCE(irqs_disabled());

 	BUG_ON(!scst_tgt);
-#if LINUX_VERSION_CODE <= KERNEL_VERSION(2, 6, 18)
-	WARN_ON(!srpt_tgt);
-	if (!srpt_tgt)
-		return -ENODEV;
-#else
-	if (WARN_ON(!srpt_tgt))
-		return -ENODEV;
-#endif
+	BUG_ON(!srpt_tgt);

 	srpt_release_sport(srpt_tgt);

@@ -3605,11 +3599,14 @@ static ssize_t show_login_info(struct kobject *kobj,
 						 tgt_kobj);
 	struct srpt_tgt *srpt_tgt = srpt_convert_scst_tgt(scst_tgt);
 	struct srpt_port *sport;
-	int i, len;
+	int i, res = -E_TGT_PRIV_NOT_YET_SET;
+
+	if (!srpt_tgt)
+		goto out;

 	if (one_target_per_port) {
 		sport = container_of(srpt_tgt, struct srpt_port, srpt_tgt);
-		len = sprintf(buf,
+		res = sprintf(buf,
 			      "tid_ext=%016llx,ioc_guid=%016llx,pkey=ffff,"
 			      "dgid=%04x%04x%04x%04x%04x%04x%04x%04x,"
 			      "service_id=%016llx\n",
@@ -3627,11 +3624,11 @@ static ssize_t show_login_info(struct kobject *kobj,
 		struct srpt_device *sdev;

 		sdev = container_of(srpt_tgt, struct srpt_device, srpt_tgt);
-		len = 0;
+		res = 0;
 		for (i = 0; i < sdev->device->phys_port_cnt; i++) {
 			sport = &sdev->port[i];

-			len += sprintf(buf + len,
+			res += sprintf(buf + res,
 				   "tid_ext=%016llx,ioc_guid=%016llx,pkey=ffff,"
 				   "dgid=%04x%04x%04x%04x%04x%04x%04x%04x,"
 				   "service_id=%016llx\n",
@@ -3649,7 +3646,8 @@ static ssize_t show_login_info(struct kobject *kobj,
 		}
 	}

-	return len;
+out:
+	return res;
 }

 static struct kobj_attribute srpt_show_login_info_attr =