* Re: [PATCH v2] st: Take additional queue ref in st_probe
2013-03-05 15:57 ` [PATCH v2] " Joe Lawrence
@ 2013-03-05 17:17 ` Kai Makisara
2013-03-05 17:34 ` James Bottomley
2013-03-15 9:13 ` Jan Vesely
2013-03-15 12:51 ` Ewan Milne
2 siblings, 1 reply; 9+ messages in thread
From: Kai Makisara @ 2013-03-05 17:17 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Jan Vesely, Seymour, Shane M, James E.J. Bottomley,
Ewan D. Milne
[-- 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] st: Take additional queue ref in st_probe
2013-03-05 17:17 ` Kai Makisara
@ 2013-03-05 17:34 ` James Bottomley
0 siblings, 0 replies; 9+ messages in thread
From: James Bottomley @ 2013-03-05 17:34 UTC (permalink / raw)
To: Kai Makisara
Cc: Joe Lawrence, linux-scsi, Jan Vesely, Seymour, Shane M,
Ewan D. Milne
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] st: Take additional queue ref in st_probe
2013-03-05 15:57 ` [PATCH v2] " Joe Lawrence
2013-03-05 17:17 ` Kai Makisara
@ 2013-03-15 9:13 ` Jan Vesely
2013-03-15 12:51 ` Ewan Milne
2 siblings, 0 replies; 9+ messages in thread
From: Jan Vesely @ 2013-03-15 9:13 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Seymour, Shane M, James E.J. Bottomley,
Kai Mäkisara, Ewan D. Milne
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] st: Take additional queue ref in st_probe
2013-03-05 15:57 ` [PATCH v2] " Joe Lawrence
2013-03-05 17:17 ` Kai Makisara
2013-03-15 9:13 ` Jan Vesely
@ 2013-03-15 12:51 ` Ewan Milne
2 siblings, 0 replies; 9+ messages in thread
From: Ewan Milne @ 2013-03-15 12:51 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-scsi, Jan Vesely, Seymour, Shane M, James E.J. Bottomley,
Kai Mäkisara
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
^ permalink raw reply [flat|nested] 9+ messages in thread