* [PATCH] scsi_dh: only attach to SCSI devices
@ 2017-02-16 7:33 Hannes Reinecke
2017-02-16 14:30 ` Bart Van Assche
2017-02-16 14:32 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Hannes Reinecke @ 2017-02-16 7:33 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Keith Busch, linux-scsi, Bart van Assche,
Hannes Reinecke, Hannes Reinecke
Any device might be setting a queuedata structure, so we need to
check if the queuedata really belongs to a SCSI device before
proceeding.
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
drivers/scsi/scsi_dh.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index b8d3b97..da104ed 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -226,7 +226,9 @@ static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
spin_lock_irqsave(q->queue_lock, flags);
sdev = q->queuedata;
- if (!sdev || !get_device(&sdev->sdev_gendev))
+ if (!sdev ||
+ !scsi_is_sdev_device(&sdev->sdev_gendev) ||
+ !get_device(&sdev->sdev_gendev))
sdev = NULL;
spin_unlock_irqrestore(q->queue_lock, flags);
--
1.8.5.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_dh: only attach to SCSI devices
2017-02-16 7:33 [PATCH] scsi_dh: only attach to SCSI devices Hannes Reinecke
@ 2017-02-16 14:30 ` Bart Van Assche
2017-02-16 14:38 ` hch
2017-02-16 14:32 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2017-02-16 14:30 UTC (permalink / raw)
To: hare, martin.petersen; +Cc: hch, keith.busch, linux-scsi, hare
On Thu, 2017-02-16 at 08:33 +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/scsi/scsi_dh.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
> index b8d3b97..da104ed 100644
> --- a/drivers/scsi/scsi_dh.c
> +++ b/drivers/scsi/scsi_dh.c
> @@ -226,7 +226,9 @@ static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
>
> spin_lock_irqsave(q->queue_lock, flags);
> sdev = q->queuedata;
> - if (!sdev || !get_device(&sdev->sdev_gendev))
> + if (!sdev ||
> + !scsi_is_sdev_device(&sdev->sdev_gendev) ||
> + !get_device(&sdev->sdev_gendev))
> sdev = NULL;
> spin_unlock_irqrestore(q->queue_lock, flags);
Hello Hannes,
Sorry but this approach looks wrong to me. A block driver can store any data
in .queuedata, even data that would cause the scsi_is_sdev_device() function
to crash.
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_dh: only attach to SCSI devices
2017-02-16 7:33 [PATCH] scsi_dh: only attach to SCSI devices Hannes Reinecke
2017-02-16 14:30 ` Bart Van Assche
@ 2017-02-16 14:32 ` Christoph Hellwig
2017-02-16 14:35 ` Bart Van Assche
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-02-16 14:32 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K. Petersen, Christoph Hellwig, Keith Busch, linux-scsi,
Bart van Assche, Hannes Reinecke
On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.
Can you add a comment next to the scsi_is_sdev_device call explaining
it? The thing is a bit of a hack, but probably the best we can do for
now.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_dh: only attach to SCSI devices
2017-02-16 14:32 ` Christoph Hellwig
@ 2017-02-16 14:35 ` Bart Van Assche
0 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2017-02-16 14:35 UTC (permalink / raw)
To: hch, hare; +Cc: keith.busch, linux-scsi, hare, martin.petersen
On Thu, 2017-02-16 at 15:32 +0100, Christoph Hellwig wrote:
> On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> > Any device might be setting a queuedata structure, so we need to
> > check if the queuedata really belongs to a SCSI device before
> > proceeding.
>
> Can you add a comment next to the scsi_is_sdev_device call explaining
> it? The thing is a bit of a hack, but probably the best we can do for
> now.
A much better approach would be to change the type of the argument of
get_sdev_from_queue().
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_dh: only attach to SCSI devices
2017-02-16 14:30 ` Bart Van Assche
@ 2017-02-16 14:38 ` hch
2017-02-16 14:38 ` Hannes Reinecke
0 siblings, 1 reply; 6+ messages in thread
From: hch @ 2017-02-16 14:38 UTC (permalink / raw)
To: Bart Van Assche; +Cc: hare, martin.petersen, hch, keith.busch, linux-scsi, hare
On Thu, Feb 16, 2017 at 02:30:34PM +0000, Bart Van Assche wrote:
> > sdev = q->queuedata;
> > - if (!sdev || !get_device(&sdev->sdev_gendev))
> > + if (!sdev ||
> > + !scsi_is_sdev_device(&sdev->sdev_gendev) ||
> > + !get_device(&sdev->sdev_gendev))
> > sdev = NULL;
> > spin_unlock_irqrestore(q->queue_lock, flags);
>
> Hello Hannes,
>
> Sorry but this approach looks wrong to me. A block driver can store any data
> in .queuedata, even data that would cause the scsi_is_sdev_device() function
> to crash.
Yes, I fear you're right. I guess we need to call into the scsi_dh_*
functions through an indirection mediated by the block layer.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi_dh: only attach to SCSI devices
2017-02-16 14:38 ` hch
@ 2017-02-16 14:38 ` Hannes Reinecke
0 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2017-02-16 14:38 UTC (permalink / raw)
To: hch, Bart Van Assche; +Cc: martin.petersen, keith.busch, linux-scsi, hare
On 02/16/2017 03:38 PM, hch@lst.de wrote:
> On Thu, Feb 16, 2017 at 02:30:34PM +0000, Bart Van Assche wrote:
>>> sdev = q->queuedata;
>>> - if (!sdev || !get_device(&sdev->sdev_gendev))
>>> + if (!sdev ||
>>> + !scsi_is_sdev_device(&sdev->sdev_gendev) ||
>>> + !get_device(&sdev->sdev_gendev))
>>> sdev = NULL;
>>> spin_unlock_irqrestore(q->queue_lock, flags);
>>
>> Hello Hannes,
>>
>> Sorry but this approach looks wrong to me. A block driver can store any data
>> in .queuedata, even data that would cause the scsi_is_sdev_device() function
>> to crash.
>
> Yes, I fear you're right. I guess we need to call into the scsi_dh_*
> functions through an indirection mediated by the block layer.
>
Okay, will be tooling it up.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-16 14:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 7:33 [PATCH] scsi_dh: only attach to SCSI devices Hannes Reinecke
2017-02-16 14:30 ` Bart Van Assche
2017-02-16 14:38 ` hch
2017-02-16 14:38 ` Hannes Reinecke
2017-02-16 14:32 ` Christoph Hellwig
2017-02-16 14:35 ` Bart Van Assche
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.