All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-02 15:00 ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-02 15:00 UTC (permalink / raw)
  To: lduncan, cleech, jejb, martin.petersen, open-iscsi
  Cc: linux-scsi, linux-kernel, Anoob Soman

When a target sends Check Condition, whilst initiator is busy xmiting
re-queued data, could lead to race between iscsi_complete_task() and
iscsi_xmit_task() and eventually crashing with the following kernel
backtrace.

[3326150.987523] ALERT: BUG: unable to handle kernel NULL pointer dereference at 0000000000000078
[3326150.987549] ALERT: IP: [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.987571] WARN: PGD 569c8067 PUD 569c9067 PMD 0
[3326150.987582] WARN: Oops: 0002 [#1] SMP
[3326150.987593] WARN: Modules linked in: tun nfsv3 nfs fscache dm_round_robin
[3326150.987762] WARN: CPU: 2 PID: 8399 Comm: kworker/u32:1 Tainted: G O 4.4.0+2 #1
[3326150.987774] WARN: Hardware name: Dell Inc. PowerEdge R720/0W7JN5, BIOS 2.5.4 01/22/2016
[3326150.987790] WARN: Workqueue: iscsi_q_13 iscsi_xmitworker [libiscsi]
[3326150.987799] WARN: task: ffff8801d50f3800 ti: ffff8801f5458000 task.ti: ffff8801f5458000
[3326150.987810] WARN: RIP: e030:[<ffffffffa05ce70d>] [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.987825] WARN: RSP: e02b:ffff8801f545bdb0 EFLAGS: 00010246
[3326150.987831] WARN: RAX: 00000000ffffffc3 RBX: ffff880282d2ab20 RCX: ffff88026b6ac480
[3326150.987842] WARN: RDX: 0000000000000000 RSI: 00000000fffffe01 RDI: ffff880282d2ab20
[3326150.987852] WARN: RBP: ffff8801f545bdc8 R08: 0000000000000000 R09: 0000000000000008
[3326150.987862] WARN: R10: 0000000000000000 R11: 000000000000fe88 R12: 0000000000000000
[3326150.987872] WARN: R13: ffff880282d2abe8 R14: ffff880282d2abd8 R15: ffff880282d2ac08
[3326150.987890] WARN: FS: 00007f5a866b4840(0000) GS:ffff88028a640000(0000) knlGS:0000000000000000
[3326150.987900] WARN: CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[3326150.987907] WARN: CR2: 0000000000000078 CR3: 0000000070244000 CR4: 0000000000042660
[3326150.987918] WARN: Stack:
[3326150.987924] WARN: ffff880282d2ad58 ffff880282d2ab20 ffff880282d2abe8 ffff8801f545be18
[3326150.987938] WARN: ffffffffa05cea90 ffff880282d2abf8 ffff88026b59cc80 ffff88026b59cc00
[3326150.987951] WARN: ffff88022acf32c0 ffff880289491800 ffff880255a80800 0000000000000400
[3326150.987964] WARN: Call Trace:
[3326150.987975] WARN: [<ffffffffa05cea90>] iscsi_xmitworker+0x2f0/0x360 [libiscsi]
[3326150.987988] WARN: [<ffffffff8108862c>] process_one_work+0x1fc/0x3b0
[3326150.987997] WARN: [<ffffffff81088f95>] worker_thread+0x2a5/0x470
[3326150.988006] WARN: [<ffffffff8159cad8>] ? __schedule+0x648/0x870
[3326150.988015] WARN: [<ffffffff81088cf0>] ? rescuer_thread+0x300/0x300
[3326150.988023] WARN: [<ffffffff8108ddf5>] kthread+0xd5/0xe0
[3326150.988031] WARN: [<ffffffff8108dd20>] ? kthread_stop+0x110/0x110
[3326150.988040] WARN: [<ffffffff815a0bcf>] ret_from_fork+0x3f/0x70
[3326150.988048] WARN: [<ffffffff8108dd20>] ? kthread_stop+0x110/0x110
[3326150.988127] ALERT: RIP [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.988138] WARN: RSP <ffff8801f545bdb0>
[3326150.988144] WARN: CR2: 0000000000000078
[3326151.020366] WARN: ---[ end trace 1c60974d4678d81b ]---

Commit 6f8830f5bbab (scsi: libiscsi: add lock around task lists to fix list
corruption regression) introduced "taskqueuelock" to fix list corruption during
the race, but this wasn't enough.

Re-setting of conn->task to NULL, could race with iscsi_xmit_task().
iscsi_complete_task()
{
    ....
    if (conn->task == task)
        conn->task = NULL;
}

conn->task in iscsi_xmit_task() could be NULL and so will be task.
__iscsi_get_task(task) will crash (NullPtr de-ref), trying to access refcount.

iscsi_xmit_task()
{
    struct iscsi_task *task = conn->task;

    __iscsi_get_task(task);
}

This commit will take extra conn->session->back_lock in iscsi_xmit_task()
to ensure iscsi_xmit_task() waits for iscsi_complete_task(), if
iscsi_complete_task() wins the race.
If iscsi_xmit_task() wins the race, iscsi_xmit_task() increments task->refcount
(__iscsi_get_task) ensuring iscsi_complete_task() will not iscsi_free_task().

Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
 drivers/scsi/libiscsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d609383..aa3be6f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
 	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
 		return -ENODATA;
 
+	spin_lock_bh(&conn->session->back_lock);
+	if (conn->task == NULL) {
+		spin_unlock_bh(&conn->session->back_lock);
+		return -ENODATA;
+	}
 	__iscsi_get_task(task);
+	spin_unlock_bh(&conn->session->back_lock);
 	spin_unlock_bh(&conn->session->frwd_lock);
 	rc = conn->session->tt->xmit_task(task);
 	spin_lock_bh(&conn->session->frwd_lock);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-02 15:00 ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-02 15:00 UTC (permalink / raw)
  To: lduncan-IBi9RG/b67k, cleech-H+wXaHxf7aLQT0dZR+AlfA,
	jejb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	martin.petersen-QHcLZuEGTsvQT0dZR+AlfA,
	open-iscsi-/JYPxA39Uh5TLH3MbocFFw
  Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Anoob Soman

When a target sends Check Condition, whilst initiator is busy xmiting
re-queued data, could lead to race between iscsi_complete_task() and
iscsi_xmit_task() and eventually crashing with the following kernel
backtrace.

[3326150.987523] ALERT: BUG: unable to handle kernel NULL pointer dereference at 0000000000000078
[3326150.987549] ALERT: IP: [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.987571] WARN: PGD 569c8067 PUD 569c9067 PMD 0
[3326150.987582] WARN: Oops: 0002 [#1] SMP
[3326150.987593] WARN: Modules linked in: tun nfsv3 nfs fscache dm_round_robin
[3326150.987762] WARN: CPU: 2 PID: 8399 Comm: kworker/u32:1 Tainted: G O 4.4.0+2 #1
[3326150.987774] WARN: Hardware name: Dell Inc. PowerEdge R720/0W7JN5, BIOS 2.5.4 01/22/2016
[3326150.987790] WARN: Workqueue: iscsi_q_13 iscsi_xmitworker [libiscsi]
[3326150.987799] WARN: task: ffff8801d50f3800 ti: ffff8801f5458000 task.ti: ffff8801f5458000
[3326150.987810] WARN: RIP: e030:[<ffffffffa05ce70d>] [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.987825] WARN: RSP: e02b:ffff8801f545bdb0 EFLAGS: 00010246
[3326150.987831] WARN: RAX: 00000000ffffffc3 RBX: ffff880282d2ab20 RCX: ffff88026b6ac480
[3326150.987842] WARN: RDX: 0000000000000000 RSI: 00000000fffffe01 RDI: ffff880282d2ab20
[3326150.987852] WARN: RBP: ffff8801f545bdc8 R08: 0000000000000000 R09: 0000000000000008
[3326150.987862] WARN: R10: 0000000000000000 R11: 000000000000fe88 R12: 0000000000000000
[3326150.987872] WARN: R13: ffff880282d2abe8 R14: ffff880282d2abd8 R15: ffff880282d2ac08
[3326150.987890] WARN: FS: 00007f5a866b4840(0000) GS:ffff88028a640000(0000) knlGS:0000000000000000
[3326150.987900] WARN: CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[3326150.987907] WARN: CR2: 0000000000000078 CR3: 0000000070244000 CR4: 0000000000042660
[3326150.987918] WARN: Stack:
[3326150.987924] WARN: ffff880282d2ad58 ffff880282d2ab20 ffff880282d2abe8 ffff8801f545be18
[3326150.987938] WARN: ffffffffa05cea90 ffff880282d2abf8 ffff88026b59cc80 ffff88026b59cc00
[3326150.987951] WARN: ffff88022acf32c0 ffff880289491800 ffff880255a80800 0000000000000400
[3326150.987964] WARN: Call Trace:
[3326150.987975] WARN: [<ffffffffa05cea90>] iscsi_xmitworker+0x2f0/0x360 [libiscsi]
[3326150.987988] WARN: [<ffffffff8108862c>] process_one_work+0x1fc/0x3b0
[3326150.987997] WARN: [<ffffffff81088f95>] worker_thread+0x2a5/0x470
[3326150.988006] WARN: [<ffffffff8159cad8>] ? __schedule+0x648/0x870
[3326150.988015] WARN: [<ffffffff81088cf0>] ? rescuer_thread+0x300/0x300
[3326150.988023] WARN: [<ffffffff8108ddf5>] kthread+0xd5/0xe0
[3326150.988031] WARN: [<ffffffff8108dd20>] ? kthread_stop+0x110/0x110
[3326150.988040] WARN: [<ffffffff815a0bcf>] ret_from_fork+0x3f/0x70
[3326150.988048] WARN: [<ffffffff8108dd20>] ? kthread_stop+0x110/0x110
[3326150.988127] ALERT: RIP [<ffffffffa05ce70d>] iscsi_xmit_task+0x2d/0xc0 [libiscsi]
[3326150.988138] WARN: RSP <ffff8801f545bdb0>
[3326150.988144] WARN: CR2: 0000000000000078
[3326151.020366] WARN: ---[ end trace 1c60974d4678d81b ]---

Commit 6f8830f5bbab (scsi: libiscsi: add lock around task lists to fix list
corruption regression) introduced "taskqueuelock" to fix list corruption during
the race, but this wasn't enough.

Re-setting of conn->task to NULL, could race with iscsi_xmit_task().
iscsi_complete_task()
{
    ....
    if (conn->task == task)
        conn->task = NULL;
}

conn->task in iscsi_xmit_task() could be NULL and so will be task.
__iscsi_get_task(task) will crash (NullPtr de-ref), trying to access refcount.

iscsi_xmit_task()
{
    struct iscsi_task *task = conn->task;

    __iscsi_get_task(task);
}

This commit will take extra conn->session->back_lock in iscsi_xmit_task()
to ensure iscsi_xmit_task() waits for iscsi_complete_task(), if
iscsi_complete_task() wins the race.
If iscsi_xmit_task() wins the race, iscsi_xmit_task() increments task->refcount
(__iscsi_get_task) ensuring iscsi_complete_task() will not iscsi_free_task().

Signed-off-by: Anoob Soman <anoob.soman-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
---
 drivers/scsi/libiscsi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d609383..aa3be6f 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
 	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
 		return -ENODATA;
 
+	spin_lock_bh(&conn->session->back_lock);
+	if (conn->task == NULL) {
+		spin_unlock_bh(&conn->session->back_lock);
+		return -ENODATA;
+	}
 	__iscsi_get_task(task);
+	spin_unlock_bh(&conn->session->back_lock);
 	spin_unlock_bh(&conn->session->frwd_lock);
 	rc = conn->session->tt->xmit_task(task);
 	spin_lock_bh(&conn->session->frwd_lock);
-- 
1.8.3.1

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-09 10:43   ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-09 10:43 UTC (permalink / raw)
  To: lduncan, cleech, jejb, martin.petersen, open-iscsi
  Cc: linux-scsi, linux-kernel

On 02/07/18 16:00, Anoob Soman wrote:
> ---
>   drivers/scsi/libiscsi.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index d609383..aa3be6f 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
>   	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
>   		return -ENODATA;
>   
> +	spin_lock_bh(&conn->session->back_lock);
> +	if (conn->task == NULL) {
> +		spin_unlock_bh(&conn->session->back_lock);
> +		return -ENODATA;
> +	}
>   	__iscsi_get_task(task);
> +	spin_unlock_bh(&conn->session->back_lock);
>   	spin_unlock_bh(&conn->session->frwd_lock);
>   	rc = conn->session->tt->xmit_task(task);
>   	spin_lock_bh(&conn->session->frwd_lock);


Hi Chris, Lee.

Could one of you look at this change and provide some comments ?

Thanks,

-Anoob.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-09 10:43   ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-09 10:43 UTC (permalink / raw)
  To: lduncan-IBi9RG/b67k, cleech-H+wXaHxf7aLQT0dZR+AlfA,
	jejb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	martin.petersen-QHcLZuEGTsvQT0dZR+AlfA,
	open-iscsi-/JYPxA39Uh5TLH3MbocFFw
  Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 02/07/18 16:00, Anoob Soman wrote:
> ---
>   drivers/scsi/libiscsi.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index d609383..aa3be6f 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
>   	if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
>   		return -ENODATA;
>   
> +	spin_lock_bh(&conn->session->back_lock);
> +	if (conn->task == NULL) {
> +		spin_unlock_bh(&conn->session->back_lock);
> +		return -ENODATA;
> +	}
>   	__iscsi_get_task(task);
> +	spin_unlock_bh(&conn->session->back_lock);
>   	spin_unlock_bh(&conn->session->frwd_lock);
>   	rc = conn->session->tt->xmit_task(task);
>   	spin_lock_bh(&conn->session->frwd_lock);


Hi Chris, Lee.

Could one of you look at this change and provide some comments ?

Thanks,

-Anoob.

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-17 15:56     ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-17 15:56 UTC (permalink / raw)
  To: lduncan, cleech, jejb, martin.petersen, open-iscsi
  Cc: linux-scsi, linux-kernel

On 09/07/18 11:43, Anoob Soman wrote:
> On 02/07/18 16:00, Anoob Soman wrote:
>> ---
>>   drivers/scsi/libiscsi.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index d609383..aa3be6f 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn 
>> *conn)
>>       if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
>>           return -ENODATA;
>>   +    spin_lock_bh(&conn->session->back_lock);
>> +    if (conn->task == NULL) {
>> +        spin_unlock_bh(&conn->session->back_lock);
>> +        return -ENODATA;
>> +    }
>>       __iscsi_get_task(task);
>> +    spin_unlock_bh(&conn->session->back_lock);
>>       spin_unlock_bh(&conn->session->frwd_lock);
>>       rc = conn->session->tt->xmit_task(task);
>>       spin_lock_bh(&conn->session->frwd_lock);
>
>
> Hi Chris, Lee.
>
> Could one of you look at this change and provide some comments ?
>
> Thanks,
>
> -Anoob.
>

Hi,

Can someone look at this change ?

Thanks,

Anoob.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
@ 2018-07-17 15:56     ` Anoob Soman
  0 siblings, 0 replies; 6+ messages in thread
From: Anoob Soman @ 2018-07-17 15:56 UTC (permalink / raw)
  To: lduncan-IBi9RG/b67k, cleech-H+wXaHxf7aLQT0dZR+AlfA,
	jejb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	martin.petersen-QHcLZuEGTsvQT0dZR+AlfA,
	open-iscsi-/JYPxA39Uh5TLH3MbocFFw
  Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA

On 09/07/18 11:43, Anoob Soman wrote:
> On 02/07/18 16:00, Anoob Soman wrote:
>> ---
>>   drivers/scsi/libiscsi.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index d609383..aa3be6f 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -1449,7 +1449,13 @@ static int iscsi_xmit_task(struct iscsi_conn 
>> *conn)
>>       if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx))
>>           return -ENODATA;
>>   +    spin_lock_bh(&conn->session->back_lock);
>> +    if (conn->task == NULL) {
>> +        spin_unlock_bh(&conn->session->back_lock);
>> +        return -ENODATA;
>> +    }
>>       __iscsi_get_task(task);
>> +    spin_unlock_bh(&conn->session->back_lock);
>>       spin_unlock_bh(&conn->session->frwd_lock);
>>       rc = conn->session->tt->xmit_task(task);
>>       spin_lock_bh(&conn->session->frwd_lock);
>
>
> Hi Chris, Lee.
>
> Could one of you look at this change and provide some comments ?
>
> Thanks,
>
> -Anoob.
>

Hi,

Can someone look at this change ?

Thanks,

Anoob.

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-17 15:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 15:00 [RFC 1/1] libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task Anoob Soman
2018-07-02 15:00 ` Anoob Soman
2018-07-09 10:43 ` Anoob Soman
2018-07-09 10:43   ` Anoob Soman
2018-07-17 15:56   ` Anoob Soman
2018-07-17 15:56     ` Anoob Soman

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.