oops in ibmvstgt

oops in ibmvstgt

[Olaf Hering]

change init order to fix crash due to uninitalized shost_data
No idea if the patch is correct.

...
running with firmware 'IBM,SF235_214' on model 'IBM,9133-55A', serial 'IBM,0210C3E0D', partition 'vioserver'
...
Linux version 2.6.28-rc7-git1 (olaf@gooseberry) (gcc version 4.1.2 20070115 (SUSE Linux)) #2 SMP Wed Dec 3 14:28:31 CET 2008
...
vio_register_driver: driver ibmvscsi registering
IBM eServer i/pSeries Virtual SCSI Target Driver
vio_register_driver: driver ibmvscsis registering
scsi1 : ibmvstgt
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc0000000002d20f4
cpu 0x0: Vector: 300 (Data Access) at [c0000001db4c7900]
    pc: c0000000002d20f4: .srp_rport_add+0xb8/0x1c4
    lr: c0000000002d20a8: .srp_rport_add+0x6c/0x1c4
    sp: c0000001db4c7b80
   msr: 8000000000009032
   dar: 0
 dsisr: 40000000
  current = 0xc0000001de40ee80
  paca    = 0xc000000000753380
    pid   = 415, comm = ibmvtgtd/0
enter ? for help
[c0000001db4c7c30] c0000000002e3f14 .process_crq+0x2d8/0x4ec
[c0000001db4c7d00] c0000000002e4394 .handle_crq+0xac/0x270
[c0000001db4c7db0] c00000000006eec0 .run_workqueue+0x114/0x204
[c0000001db4c7e50] c0000000000700cc .worker_thread+0x118/0x138
[c0000001db4c7f00] c0000000000740bc .kthread+0x78/0xc4
[c0000001db4c7f90] c000000000028ff4 .kernel_thread+0x54/0x70
...
---
 drivers/scsi/ibmvscsi/ibmvstgt.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/scsi/ibmvscsi/ibmvstgt.c
+++ b/drivers/scsi/ibmvscsi/ibmvstgt.c
@@ -864,14 +864,14 @@ static int ibmvstgt_probe(struct vio_dev
 
 	INIT_WORK(&vport->crq_work, handle_crq);
 
-	err = crq_queue_create(&vport->crq_queue, target);
-	if (err)
-		goto free_srp_target;
-
 	err = scsi_add_host(shost, target->dev);
 	if (err)
 		goto destroy_queue;
 
+	err = crq_queue_create(&vport->crq_queue, target);
+	if (err)
+		goto free_srp_target;
+
 	err = scsi_tgt_alloc_queue(shost);
 	if (err)
 		goto destroy_queue;

Re: oops in ibmvstgt

[FUJITA Tomonori]

On Wed, 3 Dec 2008 15:58:57 +0100
Olaf Hering <olh@suse.de> wrote:

> 
> change init order to fix crash due to uninitalized shost_data

I think that calling crq_queue_create leads to the creation of a rport
so we need to set up everything before that. The current code is racy.


> No idea if the patch is correct.

Almost correct, I think. Needs to modify the error handling
too. Probably, it would be better to call scsi_tgt_alloc_queue before
crq_queue_create.

How about this?

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] ibmvstgt: move crq_queue_create at the end of initialization

Calling crq_queue_create could lead to the creation of a rport. We
need to set up everything before creating a rport. This moves
crq_queue_create at the end of initialization to avoid a race.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/ibmvscsi/ibmvstgt.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvstgt.c b/drivers/scsi/ibmvscsi/ibmvstgt.c
index 2a5b29d..8dcb59e 100644
--- a/drivers/scsi/ibmvscsi/ibmvstgt.c
+++ b/drivers/scsi/ibmvscsi/ibmvstgt.c
@@ -864,21 +864,19 @@ static int ibmvstgt_probe(struct vio_dev *dev, const struct vio_device_id *id)
 
 	INIT_WORK(&vport->crq_work, handle_crq);
 
-	err = crq_queue_create(&vport->crq_queue, target);
+	err = scsi_add_host(shost, target->dev);
 	if (err)
 		goto free_srp_target;
 
-	err = scsi_add_host(shost, target->dev);
+	err = scsi_tgt_alloc_queue(shost);
 	if (err)
-		goto destroy_queue;
+		goto free_srp_target;
 
-	err = scsi_tgt_alloc_queue(shost);
+	err = crq_queue_create(&vport->crq_queue, target);
 	if (err)
-		goto destroy_queue;
+		goto free_srp_target;
 
 	return 0;
-destroy_queue:
-	crq_queue_destroy(target);
 free_srp_target:
 	srp_target_free(target);
 put_host:
-- 
1.5.5.GIT

Re: oops in ibmvstgt

[Olaf Hering]

On Thu, Dec 04, FUJITA Tomonori wrote:

> On Wed, 3 Dec 2008 15:58:57 +0100
> Olaf Hering <olh@suse.de> wrote:
> 
> > 
> > change init order to fix crash due to uninitalized shost_data
> 
> I think that calling crq_queue_create leads to the creation of a rport
> so we need to set up everything before that. The current code is racy.
> 
> 
> > No idea if the patch is correct.
> 
> Almost correct, I think. Needs to modify the error handling
> too. Probably, it would be better to call scsi_tgt_alloc_queue before
> crq_queue_create.
> 
> How about this?

Yes, that works for me.
Would be nice to get this into 2.6.28 to avoid a crash.

Olaf

Re: oops in ibmvstgt

[FUJITA Tomonori]

On Fri, 5 Dec 2008 11:53:09 +0100
Olaf Hering <olh@suse.de> wrote:

> On Thu, Dec 04, FUJITA Tomonori wrote:
> 
> > On Wed, 3 Dec 2008 15:58:57 +0100
> > Olaf Hering <olh@suse.de> wrote:
> > 
> > > 
> > > change init order to fix crash due to uninitalized shost_data
> > 
> > I think that calling crq_queue_create leads to the creation of a rport
> > so we need to set up everything before that. The current code is racy.
> > 
> > 
> > > No idea if the patch is correct.
> > 
> > Almost correct, I think. Needs to modify the error handling
> > too. Probably, it would be better to call scsi_tgt_alloc_queue before
> > crq_queue_create.
> > 
> > How about this?
> 
> Yes, that works for me.

Thanks!

> Would be nice to get this into 2.6.28 to avoid a crash.

Yeah, hopefully we can. I'll resend this with a proper description.

Re: oops in ibmvstgt

[Brian King]

FUJITA Tomonori wrote:
> On Wed, 3 Dec 2008 15:58:57 +0100
> Olaf Hering <olh@suse.de> wrote:
> 
>> change init order to fix crash due to uninitalized shost_data
> 
> I think that calling crq_queue_create leads to the creation of a rport
> so we need to set up everything before that. The current code is racy.
> 
> 
>> No idea if the patch is correct.
> 
> Almost correct, I think. Needs to modify the error handling
> too. Probably, it would be better to call scsi_tgt_alloc_queue before
> crq_queue_create.
> 
> How about this?

Don't we need to do a little more in the error handling like this:

-Brian

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] ibmvstgt: move crq_queue_create at the end of initialization

Calling crq_queue_create could lead to the creation of a rport. We
need to set up everything before creating a rport. This moves
crq_queue_create at the end of initialization to avoid a race.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---

index 2a5b29d..8dcb59e 100644
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 drivers/scsi/ibmvscsi/ibmvstgt.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff -puN drivers/scsi/ibmvscsi/ibmvstgt.c~scsi_oops_in_ibmvstgt drivers/scsi/ibmvscsi/ibmvstgt.c
--- linux-2.6/drivers/scsi/ibmvscsi/ibmvstgt.c~scsi_oops_in_ibmvstgt	2008-12-08 13:04:21.000000000 -0600
+++ linux-2.6-bjking1/drivers/scsi/ibmvscsi/ibmvstgt.c	2008-12-08 13:16:32.000000000 -0600
@@ -864,21 +864,23 @@ static int ibmvstgt_probe(struct vio_dev
 
 	INIT_WORK(&vport->crq_work, handle_crq);
 
-	err = crq_queue_create(&vport->crq_queue, target);
+	err = scsi_add_host(shost, target->dev);
 	if (err)
 		goto free_srp_target;
 
-	err = scsi_add_host(shost, target->dev);
+	err = scsi_tgt_alloc_queue(shost);
 	if (err)
-		goto destroy_queue;
+		goto remove_host;
 
-	err = scsi_tgt_alloc_queue(shost);
+	err = crq_queue_create(&vport->crq_queue, target);
 	if (err)
-		goto destroy_queue;
+		goto free_queue;
 
 	return 0;
-destroy_queue:
-	crq_queue_destroy(target);
+free_queue:
+	scsi_tgt_free_queue(shost);
+remove_host:
+	scsi_remove_host(shost);
 free_srp_target:
 	srp_target_free(target);
 put_host:
_

Re: oops in ibmvstgt

[FUJITA Tomonori]

On Mon, 08 Dec 2008 13:21:50 -0600
Brian King <brking@linux.vnet.ibm.com> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 3 Dec 2008 15:58:57 +0100
> > Olaf Hering <olh@suse.de> wrote:
> > 
> >> change init order to fix crash due to uninitalized shost_data
> > 
> > I think that calling crq_queue_create leads to the creation of a rport
> > so we need to set up everything before that. The current code is racy.
> > 
> > 
> >> No idea if the patch is correct.
> > 
> > Almost correct, I think. Needs to modify the error handling
> > too. Probably, it would be better to call scsi_tgt_alloc_queue before
> > crq_queue_create.
> > 
> > How about this?
> 
> Don't we need to do a little more in the error handling like this:

Duh, you are right. Thanks a lot!

I slightly modified the description.

=
From: Brian King <brking@linux.vnet.ibm.com>
Subject: [PATCH] ibmvstgt: move crq_queue_create at the end of initialization

Calling crq_queue_create could lead to the creation of a rport. We
need to set up everything before creating a rport. This moves
crq_queue_create at the end of initialization to avoid a race.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Reported-by: Olaf Hering <olh@suse.de>
Cc: stable@kernel.org
---
 drivers/scsi/ibmvscsi/ibmvstgt.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvstgt.c b/drivers/scsi/ibmvscsi/ibmvstgt.c
index 2a5b29d..e2dd6a4 100644
--- a/drivers/scsi/ibmvscsi/ibmvstgt.c
+++ b/drivers/scsi/ibmvscsi/ibmvstgt.c
@@ -864,21 +864,23 @@ static int ibmvstgt_probe(struct vio_dev *dev, const struct vio_device_id *id)
 
 	INIT_WORK(&vport->crq_work, handle_crq);
 
-	err = crq_queue_create(&vport->crq_queue, target);
+	err = scsi_add_host(shost, target->dev);
 	if (err)
 		goto free_srp_target;
 
-	err = scsi_add_host(shost, target->dev);
+	err = scsi_tgt_alloc_queue(shost);
 	if (err)
-		goto destroy_queue;
+		goto remove_host;
 
-	err = scsi_tgt_alloc_queue(shost);
+	err = crq_queue_create(&vport->crq_queue, target);
 	if (err)
-		goto destroy_queue;
+		goto free_queue;
 
 	return 0;
-destroy_queue:
-	crq_queue_destroy(target);
+free_queue:
+	scsi_tgt_free_queue(shost);
+remove_host:
+	scsi_remove_host(shost);
 free_srp_target:
 	srp_target_free(target);
 put_host:
-- 
1.5.5.GIT