Skip to contentSkip to conversation

IB/iser: Fix connection teardown race condition

PATCH In review Leon Romanovsky 4 participants 4 messages May 21, 2017 – Jul 22, 2017 1 message not in the archive

Subsystems (3)
Stable branch InfiniBand iSCSI extensions for RDMA (iSER) initiator
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

PATCH 1/1 IB/iser: Fix connection teardown race condition

From: Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/[email protected]>

Under heavy iser target(scst) start/stop stress during login/logout on iser intitiator side happened trace call provided below.

The function iscsi_iser_slave_alloc iser_conn pointer could be NULL, due to the fact that function iscsi_iser_conn_stop can be called before and free iser connection. Let's protect that flow by introducing global mutex.

BUG: unable to handle kernel paging request at 0000000000001018
IP: [<ffffffffc0426f7e>] iscsi_iser_slave_alloc+0x1e/0x50 [ib_iser]
Call Trace:
? scsi_alloc_sdev+0x242/0x300
scsi_probe_and_add_lun+0x9e1/0xea0
? kfree_const+0x21/0x30
? kobject_set_name_vargs+0x76/0x90
? __pm_runtime_resume+0x5b/0x70
__scsi_scan_target+0xf6/0x250
scsi_scan_target+0xea/0x100
iscsi_user_scan_session.part.13+0x101/0x130 [scsi_transport_iscsi]
? iscsi_user_scan_session.part.13+0x130/0x130 [scsi_transport_iscsi]
iscsi_user_scan_session+0x1e/0x30 [scsi_transport_iscsi]
device_for_each_child+0x50/0x90
iscsi_user_scan+0x44/0x60 [scsi_transport_iscsi]
store_scan+0xa8/0x100
? common_file_perm+0x5d/0x1c0
dev_attr_store+0x18/0x30
sysfs_kf_write+0x37/0x40
kernfs_fop_write+0x12c/0x1c0
__vfs_write+0x18/0x40
vfs_write+0xb5/0x1a0
SyS_write+0x55/0xc0
Fixes 318d311e8f01 ("iser: Accept arbitrary sg lists mapping if the device supports it")
Cc <[email protected]> # v4.5+
Signed-off-by Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/infiniband/ulp/iser/iscsi_iser.c +11 -0
index 5a887efb4bdf..9ba649836d86 100644
@@ -83,6 +83,7 @@ static struct scsi_host_template iscsi_iser_sht;
 static struct iscsi_transport iscsi_iser_transport;
 static struct scsi_transport_template *iscsi_iser_scsi_transport;
 static struct workqueue_struct *release_wq;
+static DEFINE_MUTEX(unbind_iser_conn_mutex);
 struct iser_global ig;
 
 int iser_debug_level = 0;

@@ -550,12 +551,14 @@ iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
 	 */
 	if (iser_conn) {
 		mutex_lock(&iser_conn->state_mutex);
+		mutex_lock(&unbind_iser_conn_mutex);
 		iser_conn_terminate(iser_conn);
 		iscsi_conn_stop(cls_conn, flag);
 
 		/* unbind */
 		iser_conn->iscsi_conn = NULL;
 		conn->dd_data = NULL;
+		mutex_unlock(&unbind_iser_conn_mutex);
 
 		complete(&iser_conn->stop_completion);
 		mutex_unlock(&iser_conn->state_mutex);

@@ -977,13 +980,21 @@ static int iscsi_iser_slave_alloc(struct scsi_device *sdev)
 	struct iser_conn *iser_conn;
 	struct ib_device *ib_dev;
 
+	mutex_lock(&unbind_iser_conn_mutex);
+
 	session = starget_to_session(scsi_target(sdev))->dd_data;
 	iser_conn = session->leadconn->dd_data;
+	if (!iser_conn) {
+		mutex_unlock(&unbind_iser_conn_mutex);
+		return -ENOTCONN;
+	}
 	ib_dev = iser_conn->ib_conn.device->ib_device;
 
 	if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG))
 		blk_queue_virt_boundary(sdev->request_queue, ~MASK_4K);
 
+	mutex_unlock(&unbind_iser_conn_mutex);
+
 	return 0;
 }

--
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#1 lore raw

Hi Sagi,

what do you think on this approach ?
we must somehow defend on the NULL deref in case dd_data in iscsi_conn (the iser_conn) already NULL due to conn_stop call.

Quoted 58 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +11 -1
index 5a887efb4bdf..9ba649836d86 100644
@@ -83,6 +83,7 @@ static struct scsi_host_template iscsi_iser_sht;
 static struct iscsi_transport iscsi_iser_transport;
 static struct scsi_transport_template *iscsi_iser_scsi_transport;
 static struct workqueue_struct *release_wq;
+static DEFINE_MUTEX(unbind_iser_conn_mutex);
 struct iser_global ig;
 
 int iser_debug_level = 0;

@@ -550,12 +551,14 @@ iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
 	 */
 	if (iser_conn) {
 		mutex_lock(&iser_conn->state_mutex);
+		mutex_lock(&unbind_iser_conn_mutex);
 		iser_conn_terminate(iser_conn);
 		iscsi_conn_stop(cls_conn, flag);
 
 		/* unbind */
 		iser_conn->iscsi_conn = NULL;
 		conn->dd_data = NULL;
+		mutex_unlock(&unbind_iser_conn_mutex);
 
 		complete(&iser_conn->stop_completion);
 		mutex_unlock(&iser_conn->state_mutex);

@@ -977,13 +980,21 @@ static int iscsi_iser_slave_alloc(struct scsi_device *sdev)
 	struct iser_conn *iser_conn;
 	struct ib_device *ib_dev;
 
+	mutex_lock(&unbind_iser_conn_mutex);
+
 	session = starget_to_session(scsi_target(sdev))->dd_data;
 	iser_conn = session->leadconn->dd_data;
+	if (!iser_conn) {
+		mutex_unlock(&unbind_iser_conn_mutex);
+		return -ENOTCONN;
+	}
 	ib_dev = iser_conn->ib_conn.device->ib_device;
 
 	if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_SG_GAPS_REG))
 		blk_queue_virt_boundary(sdev->request_queue, ~MASK_4K);
 
+	mutex_unlock(&unbind_iser_conn_mutex);
+
 	return 0;
 }
 
--

2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer Reviewed-by
#2 lore raw
Quoted 6 lines

Hi Sagi,

what do you think on this approach ?
we must somehow defend on the NULL deref in case dd_data in iscsi_conn (the iser_conn) already NULL due to conn_stop call.

Sounds like we get conn_stop duing lun scanning. Generally speaking I don't like a global lock for this, but given the non trivial iscsi state machine we can do with that.

Reviewed-by Sagi Grimberg <[email protected]>

A message that is not in the archive (sent off-list or not yet received): fa976fe5-be25-1c6a-5997-8fa7524e89d8-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org

#4 lore raw
Quoted 7 lines · Sagi Grimberg
Quoted 6 lines

Hi Sagi,

what do you think on this approach ?
we must somehow defend on the NULL deref in case dd_data in iscsi_conn (the iser_conn) already NULL due to conn_stop call.

Quoted 6 lines

Sounds like we get conn_stop duing lun scanning. Generally speaking I don't like a global lock for this, but given the non trivial iscsi state machine
we can do with that.

Reviewed-by Sagi Grimberg <sagi-egDjqUIXVlxBDLzU/[email protected]>

This patch was accepted into 4.13-rc, thanks.

Signature · 3 lines
Doug Ledford <[email protected]>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD