The scsi_end_request() function calls scsi_cmd_to_driver() indirectly and hence needs the disk->private_data pointer. Avoid that that pointer is cleared before all affected I/O requests have finished. This patch avoids that the following crash occurs:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 Call trace: scsi_mq_uninit_cmd+0x1c/0x30 scsi_end_request+0x7c/0x1b8 scsi_io_completion+0x464/0x668 scsi_finish_command+0xbc/0x160 scsi_eh_flush_done_q+0x10c/0x170 sas_scsi_recover_host+0x84c/0xa98 [libsas] scsi_error_handler+0x140/0x5b0 kthread+0x100/0x12c ret_from_fork+0x10/0x18
Cc: Christoph Hellwig hch@lst.de Cc: Ming Lei ming.lei@redhat.com Cc: Hannes Reinecke hare@suse.com Cc: Johannes Thumshirn jthumshirn@suse.de Cc: Jason Yan yanaijie@huawei.com Cc: stable@vger.kernel.org Reported-by: Jason Yan yanaijie@huawei.com Signed-off-by: Bart Van Assche bvanassche@acm.org --- drivers/scsi/sd.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ed34bfbc3844..0077880c0cc8 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1416,11 +1416,6 @@ static void sd_release(struct gendisk *disk, fmode_t mode) scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW); }
- /* - * XXX and what if there are packets in flight and this close() - * XXX is followed by a "rmmod sd_mod"? - */ - scsi_disk_put(sdkp); }
@@ -3483,9 +3478,21 @@ static void scsi_disk_release(struct device *dev) { struct scsi_disk *sdkp = to_scsi_disk(dev); struct gendisk *disk = sdkp->disk; - + struct request_queue *q = disk->queue; + ida_free(&sd_index_ida, sdkp->index);
+ /* + * Wait until all requests that are in progress have completed. + * This is necessary to avoid that e.g. scsi_end_request() crashes + * due to clearing the disk->private_data pointer. Wait from inside + * scsi_disk_release() instead of from sd_release() to avoid that + * freezing and unfreezing the request queue affects user space I/O + * in case multiple processes open a /dev/sd... node concurrently. + */ + blk_mq_freeze_queue(q); + blk_mq_unfreeze_queue(q); + disk->private_data = NULL; put_disk(disk); put_device(&sdkp->device->sdev_gendev);
On Mon, Mar 25, 2019 at 10:01:46AM -0700, Bart Van Assche wrote:
The scsi_end_request() function calls scsi_cmd_to_driver() indirectly and hence needs the disk->private_data pointer. Avoid that that pointer is cleared before all affected I/O requests have finished. This patch avoids that the following crash occurs:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 Call trace: scsi_mq_uninit_cmd+0x1c/0x30 scsi_end_request+0x7c/0x1b8 scsi_io_completion+0x464/0x668 scsi_finish_command+0xbc/0x160 scsi_eh_flush_done_q+0x10c/0x170 sas_scsi_recover_host+0x84c/0xa98 [libsas] scsi_error_handler+0x140/0x5b0 kthread+0x100/0x12c ret_from_fork+0x10/0x18
Cc: Christoph Hellwig hch@lst.de Cc: Ming Lei ming.lei@redhat.com Cc: Hannes Reinecke hare@suse.com Cc: Johannes Thumshirn jthumshirn@suse.de Cc: Jason Yan yanaijie@huawei.com Cc: stable@vger.kernel.org Reported-by: Jason Yan yanaijie@huawei.com Signed-off-by: Bart Van Assche bvanassche@acm.org
drivers/scsi/sd.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ed34bfbc3844..0077880c0cc8 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1416,11 +1416,6 @@ static void sd_release(struct gendisk *disk, fmode_t mode) scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW); }
- /*
* XXX and what if there are packets in flight and this close()* XXX is followed by a "rmmod sd_mod"?*/- scsi_disk_put(sdkp);
} @@ -3483,9 +3478,21 @@ static void scsi_disk_release(struct device *dev) { struct scsi_disk *sdkp = to_scsi_disk(dev); struct gendisk *disk = sdkp->disk;
- struct request_queue *q = disk->queue;
- ida_free(&sd_index_ida, sdkp->index);
- /*
* Wait until all requests that are in progress have completed.* This is necessary to avoid that e.g. scsi_end_request() crashes* due to clearing the disk->private_data pointer. Wait from inside* scsi_disk_release() instead of from sd_release() to avoid that* freezing and unfreezing the request queue affects user space I/O* in case multiple processes open a /dev/sd... node concurrently.*/- blk_mq_freeze_queue(q);
- blk_mq_unfreeze_queue(q);
- disk->private_data = NULL; put_disk(disk); put_device(&sdkp->device->sdev_gendev);
No, this way may cause big performance issue, see my previous comment:
https://marc.info/?l=linux-scsi&m=155321977714715&w=2
Thanks, Ming
On 3/25/19 6:44 PM, Ming Lei wrote:
On Mon, Mar 25, 2019 at 10:01:46AM -0700, Bart Van Assche wrote:
The scsi_end_request() function calls scsi_cmd_to_driver() indirectly and hence needs the disk->private_data pointer. Avoid that that pointer is cleared before all affected I/O requests have finished. This patch avoids that the following crash occurs:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 Call trace: scsi_mq_uninit_cmd+0x1c/0x30 scsi_end_request+0x7c/0x1b8 scsi_io_completion+0x464/0x668 scsi_finish_command+0xbc/0x160 scsi_eh_flush_done_q+0x10c/0x170 sas_scsi_recover_host+0x84c/0xa98 [libsas] scsi_error_handler+0x140/0x5b0 kthread+0x100/0x12c ret_from_fork+0x10/0x18
Cc: Christoph Hellwig hch@lst.de Cc: Ming Lei ming.lei@redhat.com Cc: Hannes Reinecke hare@suse.com Cc: Johannes Thumshirn jthumshirn@suse.de Cc: Jason Yan yanaijie@huawei.com Cc: stable@vger.kernel.org Reported-by: Jason Yan yanaijie@huawei.com Signed-off-by: Bart Van Assche bvanassche@acm.org
drivers/scsi/sd.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ed34bfbc3844..0077880c0cc8 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1416,11 +1416,6 @@ static void sd_release(struct gendisk *disk, fmode_t mode) scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW); }
- /*
* XXX and what if there are packets in flight and this close()* XXX is followed by a "rmmod sd_mod"?*/- scsi_disk_put(sdkp); }
@@ -3483,9 +3478,21 @@ static void scsi_disk_release(struct device *dev) { struct scsi_disk *sdkp = to_scsi_disk(dev); struct gendisk *disk = sdkp->disk;
- struct request_queue *q = disk->queue;
- ida_free(&sd_index_ida, sdkp->index);
- /*
* Wait until all requests that are in progress have completed.* This is necessary to avoid that e.g. scsi_end_request() crashes* due to clearing the disk->private_data pointer. Wait from inside* scsi_disk_release() instead of from sd_release() to avoid that* freezing and unfreezing the request queue affects user space I/O* in case multiple processes open a /dev/sd... node concurrently.*/- blk_mq_freeze_queue(q);
- blk_mq_unfreeze_queue(q);
- disk->private_data = NULL; put_disk(disk); put_device(&sdkp->device->sdev_gendev);
No, this way may cause big performance issue, see my previous comment:
Have you had a look at this patch? Your comment applies to the previous version of this patch. I don't think that it applies to the current version.
Bart.
On Mon, Mar 25, 2019 at 06:56:28PM -0700, Bart Van Assche wrote:
On 3/25/19 6:44 PM, Ming Lei wrote:
On Mon, Mar 25, 2019 at 10:01:46AM -0700, Bart Van Assche wrote:
The scsi_end_request() function calls scsi_cmd_to_driver() indirectly and hence needs the disk->private_data pointer. Avoid that that pointer is cleared before all affected I/O requests have finished. This patch avoids that the following crash occurs:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 Call trace: scsi_mq_uninit_cmd+0x1c/0x30 scsi_end_request+0x7c/0x1b8 scsi_io_completion+0x464/0x668 scsi_finish_command+0xbc/0x160 scsi_eh_flush_done_q+0x10c/0x170 sas_scsi_recover_host+0x84c/0xa98 [libsas] scsi_error_handler+0x140/0x5b0 kthread+0x100/0x12c ret_from_fork+0x10/0x18
Cc: Christoph Hellwig hch@lst.de Cc: Ming Lei ming.lei@redhat.com Cc: Hannes Reinecke hare@suse.com Cc: Johannes Thumshirn jthumshirn@suse.de Cc: Jason Yan yanaijie@huawei.com Cc: stable@vger.kernel.org Reported-by: Jason Yan yanaijie@huawei.com Signed-off-by: Bart Van Assche bvanassche@acm.org
drivers/scsi/sd.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ed34bfbc3844..0077880c0cc8 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1416,11 +1416,6 @@ static void sd_release(struct gendisk *disk, fmode_t mode) scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW); }
- /*
* XXX and what if there are packets in flight and this close()* XXX is followed by a "rmmod sd_mod"?*/- scsi_disk_put(sdkp); }
@@ -3483,9 +3478,21 @@ static void scsi_disk_release(struct device *dev) { struct scsi_disk *sdkp = to_scsi_disk(dev); struct gendisk *disk = sdkp->disk;
- struct request_queue *q = disk->queue;
- ida_free(&sd_index_ida, sdkp->index);
- /*
* Wait until all requests that are in progress have completed.* This is necessary to avoid that e.g. scsi_end_request() crashes* due to clearing the disk->private_data pointer. Wait from inside* scsi_disk_release() instead of from sd_release() to avoid that* freezing and unfreezing the request queue affects user space I/O* in case multiple processes open a /dev/sd... node concurrently.*/- blk_mq_freeze_queue(q);
- blk_mq_unfreeze_queue(q);
- disk->private_data = NULL; put_disk(disk); put_device(&sdkp->device->sdev_gendev);
No, this way may cause big performance issue, see my previous comment:
Have you had a look at this patch? Your comment applies to the previous version of this patch. I don't think that it applies to the current version.
OK, sorry for missing that, then this patch looks fine.
It is still a bit over-kill for passthrough IO, but seems not a big deal.
Thanks, Ming
Bart,
The scsi_end_request() function calls scsi_cmd_to_driver() indirectly and hence needs the disk->private_data pointer. Avoid that that pointer is cleared before all affected I/O requests have finished. This patch avoids that the following crash occurs:
Applied to 5.1/scsi-fixes, thanks!
linux-stable-mirror@lists.linaro.org
-
Bart Van Assche -
Christoph Hellwig -
Martin K. Petersen -
Ming Lei