Commit 67bd94130015c507011af37858989b199c52e1de
Committed by
James Bottomley
James Bottomley
1 parent
e81ca6fe85
[SCSI] Fix device removal NULL pointer dereference
Use blk_queue_dead() to test whether the queue is dead instead of !sdev. Since scsi_prep_fn() may be invoked concurrently with __scsi_remove_device(), keep the queuedata (sdev) pointer in __scsi_remove_device(). This patch fixes a kernel oops that can be triggered by USB device removal. See also http://www.spinics.net/lists/linux-scsi/msg56254.html. Other changes included in this patch: - Swap the blk_cleanup_queue() and kfree() calls in scsi_host_dev_release() to make that code easier to grasp. - Remove the queue dead check from scsi_run_queue() since the queue state can change anyway at any point in that function where the queue lock is not held. - Remove the queue dead check from the start of scsi_request_fn() since it is redundant with the scsi_device_online() check. Reported-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu> Reviewed-by: Tejun Heo <tj@kernel.org> Cc: <stable@kernel.org> Signed-off-by: James Bottomley <JBottomley@Parallels.com>
Showing 4 changed files with 9 additions and 36 deletions Side-by-side Diff
drivers/scsi/hosts.c
| ... | ... | @@ -290,6 +290,7 @@ |
| 290 | 290 | struct Scsi_Host *shost = dev_to_shost(dev); |
| 291 | 291 | struct device *parent = dev->parent; |
| 292 | 292 | struct request_queue *q; |
| 293 | + void *queuedata; | |
| 293 | 294 | |
| 294 | 295 | scsi_proc_hostdir_rm(shost->hostt); |
| 295 | 296 | |
| ... | ... | @@ -299,9 +300,9 @@ |
| 299 | 300 | destroy_workqueue(shost->work_q); |
| 300 | 301 | q = shost->uspace_req_q; |
| 301 | 302 | if (q) { |
| 302 | - kfree(q->queuedata); | |
| 303 | - q->queuedata = NULL; | |
| 304 | - scsi_free_queue(q); | |
| 303 | + queuedata = q->queuedata; | |
| 304 | + blk_cleanup_queue(q); | |
| 305 | + kfree(queuedata); | |
| 305 | 306 | } |
| 306 | 307 | |
| 307 | 308 | scsi_destroy_command_freelist(shost); |
drivers/scsi/scsi_lib.c
| ... | ... | @@ -406,10 +406,6 @@ |
| 406 | 406 | LIST_HEAD(starved_list); |
| 407 | 407 | unsigned long flags; |
| 408 | 408 | |
| 409 | - /* if the device is dead, sdev will be NULL, so no queue to run */ | |
| 410 | - if (!sdev) | |
| 411 | - return; | |
| 412 | - | |
| 413 | 409 | shost = sdev->host; |
| 414 | 410 | if (scsi_target(sdev)->single_lun) |
| 415 | 411 | scsi_single_lun_run(sdev); |
| 416 | 412 | |
| ... | ... | @@ -1371,16 +1367,16 @@ |
| 1371 | 1367 | * may be changed after request stacking drivers call the function, |
| 1372 | 1368 | * regardless of taking lock or not. |
| 1373 | 1369 | * |
| 1374 | - * When scsi can't dispatch I/Os anymore and needs to kill I/Os | |
| 1375 | - * (e.g. !sdev), scsi needs to return 'not busy'. | |
| 1376 | - * Otherwise, request stacking drivers may hold requests forever. | |
| 1370 | + * When scsi can't dispatch I/Os anymore and needs to kill I/Os scsi | |
| 1371 | + * needs to return 'not busy'. Otherwise, request stacking drivers | |
| 1372 | + * may hold requests forever. | |
| 1377 | 1373 | */ |
| 1378 | 1374 | static int scsi_lld_busy(struct request_queue *q) |
| 1379 | 1375 | { |
| 1380 | 1376 | struct scsi_device *sdev = q->queuedata; |
| 1381 | 1377 | struct Scsi_Host *shost; |
| 1382 | 1378 | |
| 1383 | - if (!sdev) | |
| 1379 | + if (blk_queue_dead(q)) | |
| 1384 | 1380 | return 0; |
| 1385 | 1381 | |
| 1386 | 1382 | shost = sdev->host; |
| ... | ... | @@ -1491,12 +1487,6 @@ |
| 1491 | 1487 | struct scsi_cmnd *cmd; |
| 1492 | 1488 | struct request *req; |
| 1493 | 1489 | |
| 1494 | - if (!sdev) { | |
| 1495 | - while ((req = blk_peek_request(q)) != NULL) | |
| 1496 | - scsi_kill_request(req, q); | |
| 1497 | - return; | |
| 1498 | - } | |
| 1499 | - | |
| 1500 | 1490 | if(!get_device(&sdev->sdev_gendev)) |
| 1501 | 1491 | /* We must be tearing the block queue down already */ |
| 1502 | 1492 | return; |
| ... | ... | @@ -1696,20 +1686,6 @@ |
| 1696 | 1686 | blk_queue_rq_timed_out(q, scsi_times_out); |
| 1697 | 1687 | blk_queue_lld_busy(q, scsi_lld_busy); |
| 1698 | 1688 | return q; |
| 1699 | -} | |
| 1700 | - | |
| 1701 | -void scsi_free_queue(struct request_queue *q) | |
| 1702 | -{ | |
| 1703 | - unsigned long flags; | |
| 1704 | - | |
| 1705 | - WARN_ON(q->queuedata); | |
| 1706 | - | |
| 1707 | - /* cause scsi_request_fn() to kill all non-finished requests */ | |
| 1708 | - spin_lock_irqsave(q->queue_lock, flags); | |
| 1709 | - q->request_fn(q); | |
| 1710 | - spin_unlock_irqrestore(q->queue_lock, flags); | |
| 1711 | - | |
| 1712 | - blk_cleanup_queue(q); | |
| 1713 | 1689 | } |
| 1714 | 1690 | |
| 1715 | 1691 | /* |
drivers/scsi/scsi_priv.h
| ... | ... | @@ -85,7 +85,6 @@ |
| 85 | 85 | extern void scsi_io_completion(struct scsi_cmnd *, unsigned int); |
| 86 | 86 | extern void scsi_run_host_queues(struct Scsi_Host *shost); |
| 87 | 87 | extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev); |
| 88 | -extern void scsi_free_queue(struct request_queue *q); | |
| 89 | 88 | extern int scsi_init_queue(void); |
| 90 | 89 | extern void scsi_exit_queue(void); |
| 91 | 90 | struct request_queue; |
drivers/scsi/scsi_sysfs.c
| ... | ... | @@ -972,11 +972,8 @@ |
| 972 | 972 | sdev->host->hostt->slave_destroy(sdev); |
| 973 | 973 | transport_destroy_device(dev); |
| 974 | 974 | |
| 975 | - /* cause the request function to reject all I/O requests */ | |
| 976 | - sdev->request_queue->queuedata = NULL; | |
| 977 | - | |
| 978 | 975 | /* Freeing the queue signals to block that we're done */ |
| 979 | - scsi_free_queue(sdev->request_queue); | |
| 976 | + blk_cleanup_queue(sdev->request_queue); | |
| 980 | 977 | put_device(dev); |
| 981 | 978 | } |
| 982 | 979 |