All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH rdma-next] IB/iser: Fix connection teardown race condition
@ 2017-05-21 16:17 ` Leon Romanovsky
  0 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2017-05-21 16:17 UTC (permalink / raw)
  To: Doug Ledford
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vladimir Neyelov,
	stable-u79uwXL29TY76Z2rM5mHXA

From: Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v4.5+
Signed-off-by: Vladimir Neyelov <vladimirn-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 5a887efb4bdf..9ba649836d86 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH rdma-next] IB/iser: Fix connection teardown race condition
@ 2017-05-21 16:17 ` Leon Romanovsky
  0 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2017-05-21 16:17 UTC (permalink / raw)
  To: Doug Ledford; +Cc: linux-rdma, Vladimir Neyelov, stable

From: Vladimir Neyelov <vladimirn@mellanox.com>

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: <stable@vger.kernel.org> # v4.5+
Signed-off-by: Vladimir Neyelov <vladimirn@mellanox.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 5a887efb4bdf..9ba649836d86 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH rdma-next] IB/iser: Fix connection teardown race condition
  2017-05-21 16:17 ` Leon Romanovsky
@ 2017-05-25 15:24   ` Max Gurtovoy
  -1 siblings, 0 replies; 7+ messages in thread
From: Max Gurtovoy @ 2017-05-25 15:24 UTC (permalink / raw)
  To: Leon Romanovsky, Doug Ledford; +Cc: linux-rdma, Vladimir Neyelov, stable, sagig

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.


>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 5a887efb4bdf..9ba649836d86 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH rdma-next] IB/iser: Fix connection teardown race condition
@ 2017-05-25 15:24   ` Max Gurtovoy
  0 siblings, 0 replies; 7+ messages in thread
From: Max Gurtovoy @ 2017-05-25 15:24 UTC (permalink / raw)
  To: Leon Romanovsky, Doug Ledford; +Cc: linux-rdma, Vladimir Neyelov, stable, sagig

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.


>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 5a887efb4bdf..9ba649836d86 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH rdma-next] IB/iser: Fix connection teardown race condition
  2017-05-25 15:24   ` Max Gurtovoy
  (?)
@ 2017-05-30 11:50   ` Sagi Grimberg
       [not found]     ` <fa976fe5-be25-1c6a-5997-8fa7524e89d8-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
  -1 siblings, 1 reply; 7+ messages in thread
From: Sagi Grimberg @ 2017-05-30 11:50 UTC (permalink / raw)
  To: Max Gurtovoy, Leon Romanovsky, Doug Ledford
  Cc: linux-rdma, Vladimir Neyelov, stable


> 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 <sagi@grimbeg.me>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH rdma-next] IB/iser: Fix connection teardown race condition
  2017-05-30 11:50   ` Sagi Grimberg
@ 2017-07-22 17:17         ` Doug Ledford
  0 siblings, 0 replies; 7+ messages in thread
From: Doug Ledford @ 2017-07-22 17:17 UTC (permalink / raw)
  To: Sagi Grimberg, Max Gurtovoy, Leon Romanovsky
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, Vladimir Neyelov,
	stable-u79uwXL29TY76Z2rM5mHXA


[-- Attachment #1.1: Type: text/plain, Size: 737 bytes --]

On 5/30/2017 7:50 AM, Sagi Grimberg wrote:
> 
>> 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 <sagi-egDjqUIXVlxBDLzU/O5InQ@public.gmane.org>

This patch was accepted into 4.13-rc, thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH rdma-next] IB/iser: Fix connection teardown race condition
@ 2017-07-22 17:17         ` Doug Ledford
  0 siblings, 0 replies; 7+ messages in thread
From: Doug Ledford @ 2017-07-22 17:17 UTC (permalink / raw)
  To: Sagi Grimberg, Max Gurtovoy, Leon Romanovsky
  Cc: linux-rdma, Vladimir Neyelov, stable


[-- Attachment #1.1: Type: text/plain, Size: 679 bytes --]

On 5/30/2017 7:50 AM, Sagi Grimberg wrote:
> 
>> 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 <sagi@grimbeg.me>

This patch was accepted into 4.13-rc, thanks.

-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-07-22 17:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-21 16:17 [PATCH rdma-next] IB/iser: Fix connection teardown race condition Leon Romanovsky
2017-05-21 16:17 ` Leon Romanovsky
2017-05-25 15:24 ` Max Gurtovoy
2017-05-25 15:24   ` Max Gurtovoy
2017-05-30 11:50   ` Sagi Grimberg
     [not found]     ` <fa976fe5-be25-1c6a-5997-8fa7524e89d8-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
2017-07-22 17:17       ` Doug Ledford
2017-07-22 17:17         ` Doug Ledford

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.