Re: [PATCH] scsi_dh: only attach to SCSI devices

2017-02-16 Thread Hannes Reinecke 
On 02/16/2017 03:38 PM, h...@lst.de wrote:
> On Thu, Feb 16, 2017 at 02:30:34PM +, Bart Van Assche wrote:
>>> sdev = q->queuedata;
>>> -   if (!sdev || !get_device(>sdev_gendev))
>>> +   if (!sdev ||
>>> +   !scsi_is_sdev_device(>sdev_gendev) ||
>>> +   !get_device(>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 ReineckeTeamlead Storage & Networking
h...@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)

Re: [PATCH] scsi_dh: only attach to SCSI devices

2017-02-16 Thread h...@lst.de 
On Thu, Feb 16, 2017 at 02:30:34PM +, Bart Van Assche wrote:
> > sdev = q->queuedata;
> > -   if (!sdev || !get_device(>sdev_gendev))
> > +   if (!sdev ||
> > +   !scsi_is_sdev_device(>sdev_gendev) ||
> > +   !get_device(>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.

Re: [PATCH] scsi_dh: only attach to SCSI devices

2017-02-16 Thread Bart Van Assche 
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.

Re: [PATCH] scsi_dh: only attach to SCSI devices

2017-02-16 Thread Christoph Hellwig 
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.

Re: [PATCH] scsi_dh: only attach to SCSI devices

2017-02-16 Thread Bart Van Assche 
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 
> ---
>  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_gendev))
> + if (!sdev ||
> + !scsi_is_sdev_device(>sdev_gendev) ||
> + !get_device(>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.

[PATCH] scsi_dh: only attach to SCSI devices

2017-02-15 Thread 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 
---
 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_gendev))
+   if (!sdev ||
+   !scsi_is_sdev_device(>sdev_gendev) ||
+   !get_device(>sdev_gendev))
sdev = NULL;
spin_unlock_irqrestore(q->queue_lock, flags);
 
-- 
1.8.5.6