From 8ce28aaba22fd92a21d0d1a4a2132515a181a344 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Wed, 12 Sep 2012 22:13:43 +0000 Subject: [PATCH] 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: [] 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:[] [] 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: [] __scst_is_relative_target_port_id_unique+0x6a/0xc0 [scst] [] gen_relative_target_port_id+0x6e/0xd0 [scst] [] scst_tgt_enable_store_work_fn+0x3a/0xe0 [scst] [] sysfs_work_thread_fn+0xc7/0x2e0 [scst] [] ? scst_tgtt_release+0x20/0x20 [scst] [] ? scst_tgtt_release+0x20/0x20 [scst] [] kthread+0x96/0xa0 [] kernel_thread_helper+0x4/0x10 [] ? kthread_worker_fn+0x180/0x180 [] ? 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 [] iscsi_is_target_enabled+0x8/0x20 [iscsi_scst] RSP CR2: 000000000000004c ---[ end trace 6311d4069703be07 ]--- Reported-by: Etienne Dechamps Signed-off-by: Bart Van Assche with some modifications git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@4515 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- fcst/ft_sess.c | 10 +++++ iscsi-scst/kernel/target.c | 24 ++++++++--- qla2x00t/qla2x00-target/qla2x00t.c | 65 +++++++++++++++++++++--------- scst/include/scst_const.h | 13 ++++++ scst_local/scst_local.c | 22 ++++++++-- srpt/src/ib_srpt.c | 36 ++++++++--------- 6 files changed, 123 insertions(+), 47 deletions(-) diff --git a/fcst/ft_sess.c b/fcst/ft_sess.c index c0e62f1ed..f1ec05c82 100644 --- a/fcst/ft_sess.c +++ b/fcst/ft_sess.c @@ -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; } diff --git a/iscsi-scst/kernel/target.c b/iscsi-scst/kernel/target.c index fdac0b7b6..a7abc6f02 100644 --- a/iscsi-scst/kernel/target.c +++ b/iscsi-scst/kernel/target.c @@ -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) diff --git a/qla2x00t/qla2x00-target/qla2x00t.c b/qla2x00t/qla2x00-target/qla2x00t.c index 33a498635..a239c0f4e 100644 --- a/qla2x00t/qla2x00-target/qla2x00t.c +++ b/qla2x00t/qla2x00-target/qla2x00t.c @@ -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); diff --git a/scst/include/scst_const.h b/scst/include/scst_const.h index 2b05b723c..f2fdc58ec 100644 --- a/scst/include/scst_const.h +++ b/scst/include/scst_const.h @@ -32,6 +32,10 @@ #endif #include +#ifndef __KERNEL__ +#include +#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 */ diff --git a/scst_local/scst_local.c b/scst_local/scst_local.c index f52dcd46b..9039424c9 100644 --- a/scst_local/scst_local.c +++ b/scst_local/scst_local.c @@ -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; } diff --git a/srpt/src/ib_srpt.c b/srpt/src/ib_srpt.c index 0ff66d35f..105c59ad5 100644 --- a/srpt/src/ib_srpt.c +++ b/srpt/src/ib_srpt.c @@ -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 =