scsi_dh: only attach to SCSI devices
PATCH 1/1 scsi_dh: only attach to SCSI devices
Any device might be setting a queuedata structure, so we need to check if the queuedata really belongs to a SCSI device before proceeding.
--- drivers/scsi/scsi_dh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/scsi_dh.c +3 -1
@@ -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);
Signature · 1 line
1.8.5.6
Quoted 14 lines · 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 <[email protected]>--- drivers/scsi/scsi_dh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/scsi/scsi_dh.c +0
-0
drivers/scsi/scsi_dh.c static struct scsi_device *get_sdev_from_queue(struct request_queue *q) · line 233
Quoted 9 lines
drivers/scsi/scsi_dh.c +3
-1@@ -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.
Quoted 10 lines · Bart Van Assche
drivers/scsi/scsi_dh.c static struct scsi_device *get_sdev_from_queue(struct request_queue *q) · line 233
Quoted 7 lines
drivers/scsi/scsi_dh.c +3
-1sdev = 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,
Quoted 3 lines
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.
Quoted 11 lines · On 02/16/2017 03:38 PM, [email protected] wrote:
Quoted 10 lines · Bart Van Assche
drivers/scsi/scsi_dh.c static struct scsi_device *get_sdev_from_queue(struct request_queue *q) · line 233
Quoted 7 lines
drivers/scsi/scsi_dh.c +3
-1sdev = 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,
Quoted 7 lines
Quoted 3 lines
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
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +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)
Quoted 3 lines · 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.
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.
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · 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.
Can you add a comment next to the
scsi_is_sdev_devicecall 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.