[PATCH] st: Take additional queue ref in st_probe

[PATCH] st: Take additional queue ref in st_probe

[Joe Lawrence]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2772 bytes --]

These changes were applied to scsi.git, branch "misc".  This patch
fixes a reference count bug in the SCSI tape driver which can be
reproduced with the following:

* Boot with slub_debug=FZPU, tape drive attached
* echo 1 > /sys/devices/... tape device pci path .../remove
* Wait for device removal
* echo 1 > /sys/kernel/slab/blkdev_queue/validate
* Slub debug complains about corrupted poison pattern

In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
add_disk() and disk_release() were modified to get/put an additional
reference on a disk queue to fix a reference counting discrepency
between bdev release and SCSI device removal.  The ST driver never
calls add_disk(), so this commit introduced an extra kref put when the
ST driver frees its struct gendisk.

Attempts were made to fix this bug at the block level [1] but later
abandoned due to floppy driver issues [2].

[1] https://lkml.org/lkml/2012/8/27/354
[2] https://lkml.org/lkml/2012/9/22/113

Regards,

-- Joe

>From f209bda3ee16a70e714b55c372ed4265bfa91784 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@stratus.com>
Date: Fri, 1 Mar 2013 17:32:30 -0500
Subject: [PATCH] st: Take additional queue ref in st_probe
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
create an instance, but does not register it via add_disk().  When the
gendisk is torn down, disk_release() is called and expects to return a
disk queue reference that add_disk() normally would have taken out. (See
commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
to st_probe().

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Tested-by: Ewan Milne <emilne@redhat.com>
Cc: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Ewan Milne <emilne@redhat.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/st.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 98156a9..eef8ddc 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4112,6 +4112,10 @@ static int st_probe(struct device *dev)
 	tpnt->disk = disk;
 	disk->private_data = &tpnt->driver;
 	disk->queue = SDp->request_queue;
+	/* SCSI tape doesn't register this gendisk via add_disk().  Manually
+	 * take queue reference that release_disk() expects. */
+	if (!blk_get_queue(disk->queue))
+		goto out_put_queue;
 	tpnt->driver = &st_template;
 
 	tpnt->device = SDp;
@@ -4222,6 +4226,8 @@ out_put_index:
 	spin_lock(&st_index_lock);
 	idr_remove(&st_index_idr, dev_num);
 	spin_unlock(&st_index_lock);
+out_put_queue:
+	blk_put_queue(disk->queue);
 out_put_disk:
 	put_disk(disk);
 	kfree(tpnt);
-- 
1.8.1.2

Re: [PATCH] st: Take additional queue ref in st_probe

[Ewan Milne]

On Mon, 2013-03-04 at 11:14 -0500, Joe Lawrence wrote:
> These changes were applied to scsi.git, branch "misc".  This patch
> fixes a reference count bug in the SCSI tape driver which can be
> reproduced with the following:
> 
> * Boot with slub_debug=FZPU, tape drive attached
> * echo 1 > /sys/devices/... tape device pci path .../remove
> * Wait for device removal
> * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> * Slub debug complains about corrupted poison pattern

The incorrect reference count fixed by this patch is almost certainly
responsible for OOPSes seen with tape devices connected using zfcp
on the s390x architecture due to a use-after-free.  I was able to
reproduce the problem with scsi_debug ptype=1 and slub_debug enabled.
So, st device support is broken.  With the patch, the problem no longer
appears.

-Ewan <emilne@redhat.com>

RE: [PATCH] st: Take additional queue ref in st_probe

[Seymour, Shane M]

With the things I've been doing in st it was easier to reboot than unload/load the st driver since it would likely cause an oops. I applied the patch and have tested it and the st module unloads/reloads with no problems now. 

Re: [PATCH] st: Take additional queue ref in st_probe

[Jan Vesely]

Hi,

I don't understand how error paths are supposed to work with this patch.
As I see it, if we fail to get a reference, the error path tries to put a 
non-existent one. moreover, error paths on lines 4188 and 4196 still goto 
out_put_disk, leaking the reference.

other than that, it fixes the load/unload cycle issue for me as well

thanks,


On 04/03/13 17:14, Joe Lawrence wrote:
> These changes were applied to scsi.git, branch "misc".  This patch
> fixes a reference count bug in the SCSI tape driver which can be
> reproduced with the following:
>
> * Boot with slub_debug=FZPU, tape drive attached
> * echo 1 > /sys/devices/... tape device pci path .../remove
> * Wait for device removal
> * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> * Slub debug complains about corrupted poison pattern
>
> In commit 523e1d39 (block: make gendisk hold a reference to its queue)
> add_disk() and disk_release() were modified to get/put an additional
> reference on a disk queue to fix a reference counting discrepency
> between bdev release and SCSI device removal.  The ST driver never
> calls add_disk(), so this commit introduced an extra kref put when the
> ST driver frees its struct gendisk.
>
> Attempts were made to fix this bug at the block level [1] but later
> abandoned due to floppy driver issues [2].
>
> [1] https://lkml.org/lkml/2012/8/27/354
> [2] https://lkml.org/lkml/2012/9/22/113
>
> Regards,
>
> -- Joe
>
>  From f209bda3ee16a70e714b55c372ed4265bfa91784 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@stratus.com>
> Date: Fri, 1 Mar 2013 17:32:30 -0500
> Subject: [PATCH] st: Take additional queue ref in st_probe
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
> create an instance, but does not register it via add_disk().  When the
> gendisk is torn down, disk_release() is called and expects to return a
> disk queue reference that add_disk() normally would have taken out. (See
> commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
> to st_probe().
>
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Tested-by: Ewan Milne <emilne@redhat.com>
> Cc: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: Ewan Milne <emilne@redhat.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>   drivers/scsi/st.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 98156a9..eef8ddc 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -4112,6 +4112,10 @@ static int st_probe(struct device *dev)
>   	tpnt->disk = disk;
>   	disk->private_data = &tpnt->driver;
>   	disk->queue = SDp->request_queue;
> +	/* SCSI tape doesn't register this gendisk via add_disk().  Manually
> +	 * take queue reference that release_disk() expects. */
> +	if (!blk_get_queue(disk->queue))
> +		goto out_put_queue;
>   	tpnt->driver = &st_template;
>
>   	tpnt->device = SDp;
> @@ -4222,6 +4226,8 @@ out_put_index:
>   	spin_lock(&st_index_lock);
>   	idr_remove(&st_index_idr, dev_num);
>   	spin_unlock(&st_index_lock);
> +out_put_queue:
> +	blk_put_queue(disk->queue);
>   out_put_disk:
>   	put_disk(disk);
>   	kfree(tpnt);
>


-- 
-----
Jan Vesely <jvesely@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] st: Take additional queue ref in st_probe

[Joe Lawrence]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3407 bytes --]

Changes from v1:
  Corrected error paths as noted by Ewan Milne and Jan Vesely.

These changes were applied to scsi.git, branch "misc".  This patch
fixes a reference count bug in the SCSI tape driver which can be
reproduced with the following:

* Boot with slub_debug=FZPU, tape drive attached
* echo 1 > /sys/devices/... tape device pci path .../remove
* Wait for device removal
* echo 1 > /sys/kernel/slab/blkdev_queue/validate
* Slub debug complains about corrupted poison pattern

In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
add_disk() and disk_release() were modified to get/put an additional
reference on a disk queue to fix a reference counting discrepency
between bdev release and SCSI device removal.  The ST driver never
calls add_disk(), so this commit introduced an extra kref put when the
ST driver frees its struct gendisk.

Attempts were made to fix this bug at the block level [1] but later
abandoned due to floppy driver issues [2].

[1] https://lkml.org/lkml/2012/8/27/354
[2] https://lkml.org/lkml/2012/9/22/113

>From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@stratus.com>
Date: Tue, 5 Mar 2013 09:30:14 -0500
Subject: [PATCH v2] st: Take additional queue ref in st_probe
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
create an instance, but does not register it via add_disk().  When the
gendisk is torn down, disk_release() is called and expects to return a
disk queue reference that add_disk() normally would have taken out. (See
commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
to st_probe().

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Tested-by: Ewan D. Milne <emilne@redhat.com> 
Cc: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: "Seymour, Shane M" <shane.seymour@hp.com>
Cc: Jan Vesely <jvesely@redhat.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/st.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 98156a9..96f3363 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4112,6 +4112,10 @@ static int st_probe(struct device *dev)
 	tpnt->disk = disk;
 	disk->private_data = &tpnt->driver;
 	disk->queue = SDp->request_queue;
+	/* SCSI tape doesn't register this gendisk via add_disk().  Manually
+	 * take queue reference that release_disk() expects. */
+	if (!blk_get_queue(disk->queue))
+		goto out_put_disk;
 	tpnt->driver = &st_template;
 
 	tpnt->device = SDp;
@@ -4181,7 +4185,7 @@ static int st_probe(struct device *dev)
 	if (!idr_pre_get(&st_index_idr, GFP_KERNEL)) {
 		pr_warn("st: idr expansion failed\n");
 		error = -ENOMEM;
-		goto out_put_disk;
+		goto out_put_queue;
 	}
 
 	spin_lock(&st_index_lock);
@@ -4189,7 +4193,7 @@ static int st_probe(struct device *dev)
 	spin_unlock(&st_index_lock);
 	if (error) {
 		pr_warn("st: idr allocation failed: %d\n", error);
-		goto out_put_disk;
+		goto out_put_queue;
 	}
 
 	if (dev_num > ST_MAX_TAPES) {
@@ -4222,6 +4226,8 @@ out_put_index:
 	spin_lock(&st_index_lock);
 	idr_remove(&st_index_idr, dev_num);
 	spin_unlock(&st_index_lock);
+out_put_queue:
+	blk_put_queue(disk->queue);
 out_put_disk:
 	put_disk(disk);
 	kfree(tpnt);
-- 
1.8.1.2

Re: [PATCH v2] st: Take additional queue ref in st_probe

[Kai Makisara]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2265 bytes --]

On Tue, 5 Mar 2013, Joe Lawrence wrote:

> Changes from v1:
>   Corrected error paths as noted by Ewan Milne and Jan Vesely.
> 
> These changes were applied to scsi.git, branch "misc".  This patch
> fixes a reference count bug in the SCSI tape driver which can be
> reproduced with the following:
> 
> * Boot with slub_debug=FZPU, tape drive attached
> * echo 1 > /sys/devices/... tape device pci path .../remove
> * Wait for device removal
> * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> * Slub debug complains about corrupted poison pattern
> 
> In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
> add_disk() and disk_release() were modified to get/put an additional
> reference on a disk queue to fix a reference counting discrepency
> between bdev release and SCSI device removal.  The ST driver never
> calls add_disk(), so this commit introduced an extra kref put when the
> ST driver frees its struct gendisk.
> 
> Attempts were made to fix this bug at the block level [1] but later
> abandoned due to floppy driver issues [2].
> 
> [1] https://lkml.org/lkml/2012/8/27/354
> [2] https://lkml.org/lkml/2012/9/22/113
> 
> From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@stratus.com>
> Date: Tue, 5 Mar 2013 09:30:14 -0500
> Subject: [PATCH v2] st: Take additional queue ref in st_probe
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
> create an instance, but does not register it via add_disk().  When the
> gendisk is torn down, disk_release() is called and expects to return a
> disk queue reference that add_disk() normally would have taken out. (See
> commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
> to st_probe().
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Tested-by: Ewan D. Milne <emilne@redhat.com> 

I don't have to like this fix but as it fixes a real problem:

Acked-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>

(It should not be necessary to get additional references outside the block 
layer to make the block layer behave.)

Thanks,
Kai

Re: [PATCH v2] st: Take additional queue ref in st_probe

[James Bottomley]

On Tue, 2013-03-05 at 19:17 +0200, Kai Makisara wrote:
> On Tue, 5 Mar 2013, Joe Lawrence wrote:
> 
> > Changes from v1:
> >   Corrected error paths as noted by Ewan Milne and Jan Vesely.
> > 
> > These changes were applied to scsi.git, branch "misc".  This patch
> > fixes a reference count bug in the SCSI tape driver which can be
> > reproduced with the following:
> > 
> > * Boot with slub_debug=FZPU, tape drive attached
> > * echo 1 > /sys/devices/... tape device pci path .../remove
> > * Wait for device removal
> > * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> > * Slub debug complains about corrupted poison pattern
> > 
> > In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
> > add_disk() and disk_release() were modified to get/put an additional
> > reference on a disk queue to fix a reference counting discrepency
> > between bdev release and SCSI device removal.  The ST driver never
> > calls add_disk(), so this commit introduced an extra kref put when the
> > ST driver frees its struct gendisk.
> > 
> > Attempts were made to fix this bug at the block level [1] but later
> > abandoned due to floppy driver issues [2].
> > 
> > [1] https://lkml.org/lkml/2012/8/27/354
> > [2] https://lkml.org/lkml/2012/9/22/113
> > 
> > From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001
> > From: Joe Lawrence <joe.lawrence@stratus.com>
> > Date: Tue, 5 Mar 2013 09:30:14 -0500
> > Subject: [PATCH v2] st: Take additional queue ref in st_probe
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> > 
> > The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
> > create an instance, but does not register it via add_disk().  When the
> > gendisk is torn down, disk_release() is called and expects to return a
> > disk queue reference that add_disk() normally would have taken out. (See
> > commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
> > to st_probe().
> > 
> > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> > Tested-by: Ewan D. Milne <emilne@redhat.com> 
> 
> I don't have to like this fix but as it fixes a real problem:
> 
> Acked-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> 
> (It should not be necessary to get additional references outside the block 
> layer to make the block layer behave.)

Agree with that.  However, from a practical point of view, alloc_disk()
has no idea what disk queue it's allocating the minors for, so on the
information we provide to block, there's no disconnect.

On the other hand, the other consequence of this situation is that
there's no current sysfs way of getting at the queue parameters because
the gendisk isn't added.  We actually need some type of

add_disk_not_really_a_disk()

Call which would tell block about the queue associated with the disk and
ask to to add the relevant non-disk sysfs parameters.

James



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

Re: [PATCH v2] st: Take additional queue ref in st_probe

[Jan Vesely]

On 05/03/13 16:57, Joe Lawrence wrote:
> Changes from v1:
>   Corrected error paths as noted by Ewan Milne and Jan Vesely.

thanks,

Acked-by: Jan Vesely <jvesely@redhat.com>

> 
> These changes were applied to scsi.git, branch "misc".  This patch
> fixes a reference count bug in the SCSI tape driver which can be
> reproduced with the following:
> 
> * Boot with slub_debug=FZPU, tape drive attached
> * echo 1 > /sys/devices/... tape device pci path .../remove
> * Wait for device removal
> * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> * Slub debug complains about corrupted poison pattern
> 
> In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
> add_disk() and disk_release() were modified to get/put an additional
> reference on a disk queue to fix a reference counting discrepency
> between bdev release and SCSI device removal.  The ST driver never
> calls add_disk(), so this commit introduced an extra kref put when the
> ST driver frees its struct gendisk.
> 
> Attempts were made to fix this bug at the block level [1] but later
> abandoned due to floppy driver issues [2].
> 
> [1] https://lkml.org/lkml/2012/8/27/354
> [2] https://lkml.org/lkml/2012/9/22/113
> 
> From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@stratus.com>
> Date: Tue, 5 Mar 2013 09:30:14 -0500
> Subject: [PATCH v2] st: Take additional queue ref in st_probe
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
> create an instance, but does not register it via add_disk().  When the
> gendisk is torn down, disk_release() is called and expects to return a
> disk queue reference that add_disk() normally would have taken out. (See
> commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
> to st_probe().
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Tested-by: Ewan D. Milne <emilne@redhat.com> 
> Cc: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: "Seymour, Shane M" <shane.seymour@hp.com>
> Cc: Jan Vesely <jvesely@redhat.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/st.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 98156a9..96f3363 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -4112,6 +4112,10 @@ static int st_probe(struct device *dev)
>  	tpnt->disk = disk;
>  	disk->private_data = &tpnt->driver;
>  	disk->queue = SDp->request_queue;
> +	/* SCSI tape doesn't register this gendisk via add_disk().  Manually
> +	 * take queue reference that release_disk() expects. */
> +	if (!blk_get_queue(disk->queue))
> +		goto out_put_disk;
>  	tpnt->driver = &st_template;
>  
>  	tpnt->device = SDp;
> @@ -4181,7 +4185,7 @@ static int st_probe(struct device *dev)
>  	if (!idr_pre_get(&st_index_idr, GFP_KERNEL)) {
>  		pr_warn("st: idr expansion failed\n");
>  		error = -ENOMEM;
> -		goto out_put_disk;
> +		goto out_put_queue;
>  	}
>  
>  	spin_lock(&st_index_lock);
> @@ -4189,7 +4193,7 @@ static int st_probe(struct device *dev)
>  	spin_unlock(&st_index_lock);
>  	if (error) {
>  		pr_warn("st: idr allocation failed: %d\n", error);
> -		goto out_put_disk;
> +		goto out_put_queue;
>  	}
>  
>  	if (dev_num > ST_MAX_TAPES) {
> @@ -4222,6 +4226,8 @@ out_put_index:
>  	spin_lock(&st_index_lock);
>  	idr_remove(&st_index_idr, dev_num);
>  	spin_unlock(&st_index_lock);
> +out_put_queue:
> +	blk_put_queue(disk->queue);
>  out_put_disk:
>  	put_disk(disk);
>  	kfree(tpnt);
> 

-- 
Jan Vesely <jvesely@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] st: Take additional queue ref in st_probe

[Ewan Milne]

On Tue, 2013-03-05 at 10:57 -0500, Joe Lawrence wrote:
> Changes from v1:
>   Corrected error paths as noted by Ewan Milne and Jan Vesely.

Acked-by: Ewan D. Milne <emilne@redhat.com>

> 
> These changes were applied to scsi.git, branch "misc".  This patch
> fixes a reference count bug in the SCSI tape driver which can be
> reproduced with the following:
> 
> * Boot with slub_debug=FZPU, tape drive attached
> * echo 1 > /sys/devices/... tape device pci path .../remove
> * Wait for device removal
> * echo 1 > /sys/kernel/slab/blkdev_queue/validate
> * Slub debug complains about corrupted poison pattern
> 
> In commit 523e1d39 (block: make gendisk hold a reference to its queue) 
> add_disk() and disk_release() were modified to get/put an additional
> reference on a disk queue to fix a reference counting discrepency
> between bdev release and SCSI device removal.  The ST driver never
> calls add_disk(), so this commit introduced an extra kref put when the
> ST driver frees its struct gendisk.
> 
> Attempts were made to fix this bug at the block level [1] but later
> abandoned due to floppy driver issues [2].
> 
> [1] https://lkml.org/lkml/2012/8/27/354
> [2] https://lkml.org/lkml/2012/9/22/113
> 
> From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@stratus.com>
> Date: Tue, 5 Mar 2013 09:30:14 -0500
> Subject: [PATCH v2] st: Take additional queue ref in st_probe
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The SCSI tape driver employs a struct gendisk, calling alloc_disk() to
> create an instance, but does not register it via add_disk().  When the
> gendisk is torn down, disk_release() is called and expects to return a
> disk queue reference that add_disk() normally would have taken out. (See
> commit 523e1d39.)  Fix the kref accounting by adding a blk_get_queue()
> to st_probe().
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Tested-by: Ewan D. Milne <emilne@redhat.com> 
> Cc: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: "Seymour, Shane M" <shane.seymour@hp.com>
> Cc: Jan Vesely <jvesely@redhat.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/st.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 98156a9..96f3363 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -4112,6 +4112,10 @@ static int st_probe(struct device *dev)
>  	tpnt->disk = disk;
>  	disk->private_data = &tpnt->driver;
>  	disk->queue = SDp->request_queue;
> +	/* SCSI tape doesn't register this gendisk via add_disk().  Manually
> +	 * take queue reference that release_disk() expects. */
> +	if (!blk_get_queue(disk->queue))
> +		goto out_put_disk;
>  	tpnt->driver = &st_template;
>  
>  	tpnt->device = SDp;
> @@ -4181,7 +4185,7 @@ static int st_probe(struct device *dev)
>  	if (!idr_pre_get(&st_index_idr, GFP_KERNEL)) {
>  		pr_warn("st: idr expansion failed\n");
>  		error = -ENOMEM;
> -		goto out_put_disk;
> +		goto out_put_queue;
>  	}
>  
>  	spin_lock(&st_index_lock);
> @@ -4189,7 +4193,7 @@ static int st_probe(struct device *dev)
>  	spin_unlock(&st_index_lock);
>  	if (error) {
>  		pr_warn("st: idr allocation failed: %d\n", error);
> -		goto out_put_disk;
> +		goto out_put_queue;
>  	}
>  
>  	if (dev_num > ST_MAX_TAPES) {
> @@ -4222,6 +4226,8 @@ out_put_index:
>  	spin_lock(&st_index_lock);
>  	idr_remove(&st_index_idr, dev_num);
>  	spin_unlock(&st_index_lock);
> +out_put_queue:
> +	blk_put_queue(disk->queue);
>  out_put_disk:
>  	put_disk(disk);
>  	kfree(tpnt);


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