From 0761df9c4b2d966da3af2ac4ee7372afa681ce63 Mon Sep 17 00:00:00 2001 From: Hannes Reinecke Date: Fri, 10 May 2013 11:06:16 +0200 Subject: [SCSI] sd: avoid deadlocks when running under multipath When multipathed systems run into an all-paths-down scenario all devices might be dropped, too. This causes 'del_gendisk' to be called, which will unregister the kobj_map->probe() function for all disk device numbers. When the device comes back the default ->probe() function is run which will call __request_module(), which will deadlock. As 'del_gendisk' typically does _not_ trigger a module unload the default ->probe() function is pointless anyway. This patch implements a dummy ->probe() function, which will just return NULL if the disk is not registered. This will avoid the deadlock. Plus it'll speed up device scanning. Signed-off-by: Hannes Reinecke Signed-off-by: James Bottomley --- drivers/scsi/sd.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) (limited to 'drivers/scsi/sd.c') diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index c1c555242d0d..a37eda9f11f5 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -503,6 +503,16 @@ static struct scsi_driver sd_template = { .eh_action = sd_eh_action, }; +/* + * Dummy kobj_map->probe function. + * The default ->probe function will call modprobe, which is + * pointless as this module is already loaded. + */ +static struct kobject *sd_default_probe(dev_t devt, int *partno, void *data) +{ + return NULL; +} + /* * Device no to disk mapping: * @@ -2970,8 +2980,10 @@ static int sd_probe(struct device *dev) static int sd_remove(struct device *dev) { struct scsi_disk *sdkp; + dev_t devt; sdkp = dev_get_drvdata(dev); + devt = disk_devt(sdkp->disk); scsi_autopm_get_device(sdkp->device); async_synchronize_full_domain(&scsi_sd_probe_domain); @@ -2981,6 +2993,9 @@ static int sd_remove(struct device *dev) del_gendisk(sdkp->disk); sd_shutdown(dev); + blk_register_region(devt, SD_MINORS, NULL, + sd_default_probe, NULL, NULL); + mutex_lock(&sd_ref_mutex); dev_set_drvdata(dev, NULL); put_device(&sdkp->dev); @@ -3124,9 +3139,13 @@ static int __init init_sd(void) SCSI_LOG_HLQUEUE(3, printk("init_sd: sd driver entry point\n")); - for (i = 0; i < SD_MAJORS; i++) - if (register_blkdev(sd_major(i), "sd") == 0) - majors++; + for (i = 0; i < SD_MAJORS; i++) { + if (register_blkdev(sd_major(i), "sd") != 0) + continue; + majors++; + blk_register_region(sd_major(i), SD_MINORS, NULL, + sd_default_probe, NULL, NULL); + } if (!majors) return -ENODEV; @@ -3185,8 +3204,10 @@ static void __exit exit_sd(void) class_unregister(&sd_disk_class); - for (i = 0; i < SD_MAJORS; i++) + for (i = 0; i < SD_MAJORS; i++) { + blk_unregister_region(sd_major(i), SD_MINORS); unregister_blkdev(sd_major(i), "sd"); + } } module_init(init_sd); -- cgit v1.2.3 From 2ee3e26c673e75c05ef8b914f54fadee3d7b9c88 Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Mon, 27 May 2013 19:07:19 +0100 Subject: [SCSI] sd: Fix parsing of 'temporary ' cache mode prefix Commit 39c60a0948cc '[SCSI] sd: fix array cache flushing bug causing performance problems' added temp as a pointer to "temporary " and used sizeof(temp) - 1 as its length. But sizeof(temp) is the size of the pointer, not the size of the string constant. Change temp to a static array so that sizeof() does what was intended. Signed-off-by: Ben Hutchings Cc: stable@vger.kernel.org Signed-off-by: James Bottomley --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/scsi/sd.c') diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index a37eda9f11f5..91e8a953d1de 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -142,7 +142,7 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr, char *buffer_data; struct scsi_mode_data data; struct scsi_sense_hdr sshdr; - const char *temp = "temporary "; + static const char temp[] = "temporary "; int len; if (sdp->type != TYPE_DISK) -- cgit v1.2.3 From 66c28f97120e8a621afd5aa7a31c4b85c547d33d Mon Sep 17 00:00:00 2001 From: "Martin K. Petersen" Date: Thu, 6 Jun 2013 22:15:55 -0400 Subject: [SCSI] sd: Update WRITE SAME heuristics SATA drives located behind a SAS controller would incorrectly receive WRITE SAME commands. Tweak the heuristics so that: - If REPORT SUPPORTED OPERATION CODES is provided we will use that to choose between WRITE SAME(16), WRITE SAME(10) and disabled. This also fixes an issue with the old code which would issue WRITE SAME(10) despite the command not being whitelisted in REPORT SUPPORTED OPERATION CODES. - If REPORT SUPPORTED OPERATION CODES is not provided we will fall back to WRITE SAME(10) unless the device has an ATA Information VPD page. The assumption is that a SATL which is smart enough to implement WRITE SAME would also provide REPORT SUPPORTED OPERATION CODES. To facilitate the new heuristics scsi_report_opcode() has been modified to so we can distinguish between "operation not supported" and "RSOC not supported". Reported-by: H. Peter Anvin Tested-by: Bernd Schubert Signed-off-by: Martin K. Petersen Cc: Signed-off-by: James Bottomley --- drivers/scsi/sd.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) (limited to 'drivers/scsi/sd.c') diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 91e8a953d1de..34da1a1acb09 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -442,8 +442,10 @@ sd_store_write_same_blocks(struct device *dev, struct device_attribute *attr, if (max == 0) sdp->no_write_same = 1; - else if (max <= SD_MAX_WS16_BLOCKS) + else if (max <= SD_MAX_WS16_BLOCKS) { + sdp->no_write_same = 0; sdkp->max_ws_blocks = max; + } sd_config_write_same(sdkp); @@ -750,7 +752,6 @@ static void sd_config_write_same(struct scsi_disk *sdkp) { struct request_queue *q = sdkp->disk->queue; unsigned int logical_block_size = sdkp->device->sector_size; - unsigned int blocks = 0; if (sdkp->device->no_write_same) { sdkp->max_ws_blocks = 0; @@ -762,18 +763,20 @@ static void sd_config_write_same(struct scsi_disk *sdkp) * blocks per I/O unless the device explicitly advertises a * bigger limit. */ - if (sdkp->max_ws_blocks == 0) - sdkp->max_ws_blocks = SD_MAX_WS10_BLOCKS; - - if (sdkp->ws16 || sdkp->max_ws_blocks > SD_MAX_WS10_BLOCKS) - blocks = min_not_zero(sdkp->max_ws_blocks, - (u32)SD_MAX_WS16_BLOCKS); - else - blocks = min_not_zero(sdkp->max_ws_blocks, - (u32)SD_MAX_WS10_BLOCKS); + if (sdkp->max_ws_blocks > SD_MAX_WS10_BLOCKS) + sdkp->max_ws_blocks = min_not_zero(sdkp->max_ws_blocks, + (u32)SD_MAX_WS16_BLOCKS); + else if (sdkp->ws16 || sdkp->ws10 || sdkp->device->no_report_opcodes) + sdkp->max_ws_blocks = min_not_zero(sdkp->max_ws_blocks, + (u32)SD_MAX_WS10_BLOCKS); + else { + sdkp->device->no_write_same = 1; + sdkp->max_ws_blocks = 0; + } out: - blk_queue_max_write_same_sectors(q, blocks * (logical_block_size >> 9)); + blk_queue_max_write_same_sectors(q, sdkp->max_ws_blocks * + (logical_block_size >> 9)); } /** @@ -2645,9 +2648,24 @@ static void sd_read_block_provisioning(struct scsi_disk *sdkp) static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer) { - if (scsi_report_opcode(sdkp->device, buffer, SD_BUF_SIZE, - WRITE_SAME_16)) + struct scsi_device *sdev = sdkp->device; + + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, INQUIRY) < 0) { + sdev->no_report_opcodes = 1; + + /* Disable WRITE SAME if REPORT SUPPORTED OPERATION + * CODES is unsupported and the device has an ATA + * Information VPD page (SAT). + */ + if (!scsi_get_vpd_page(sdev, 0x89, buffer, SD_BUF_SIZE)) + sdev->no_write_same = 1; + } + + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME_16) == 1) sdkp->ws16 = 1; + + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE, WRITE_SAME) == 1) + sdkp->ws10 = 1; } static int sd_try_extended_inquiry(struct scsi_device *sdp) -- cgit v1.2.3 From 02aa2a37636c8fa4fb9322d91be46ff8225b7de0 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 3 Jul 2013 15:04:56 -0700 Subject: drivers: avoid format string in dev_set_name Calling dev_set_name with a single paramter causes it to be handled as a format string. Many callers are passing potentially dynamic string content, so use "%s" in those cases to avoid any potential accidents, including wrappers like device_create*() and bdi_register(). Signed-off-by: Kees Cook Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/scsi/sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/scsi/sd.c') diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index c1c555242d0d..8fa3d0b73ad9 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2931,7 +2931,7 @@ static int sd_probe(struct device *dev) device_initialize(&sdkp->dev); sdkp->dev.parent = dev; sdkp->dev.class = &sd_disk_class; - dev_set_name(&sdkp->dev, dev_name(dev)); + dev_set_name(&sdkp->dev, "%s", dev_name(dev)); if (device_add(&sdkp->dev)) goto out_free_index; -- cgit v1.2.3 From 085b513f97d8d799d28491239be4b451bcd8c2c5 Mon Sep 17 00:00:00 2001 From: "Ewan D. Milne" Date: Fri, 2 Nov 2012 09:38:34 -0400 Subject: [SCSI] sd: fix crash when UA received on DIF enabled device sd_prep_fn will allocate a larger CDB for the command via mempool_alloc for devices using DIF type 2 protection. This CDB was being freed in sd_done, which results in a kernel crash if the command is retried due to a UNIT ATTENTION. This change moves the code to free the larger CDB into sd_unprep_fn instead, which is invoked after the request is complete. It is no longer necessary to call scsi_print_command separately for this case as the ->cmnd will no longer be NULL in the normal code path. Also removed conditional test for DIF type 2 when freeing the larger CDB because the protection_type could have been changed via sysfs while the command was executing. Signed-off-by: Ewan D. Milne Acked-by: Martin K. Petersen Cc: Signed-off-by: James Bottomley --- drivers/scsi/sd.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'drivers/scsi/sd.c') diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 80f39b8b0223..86fcf2c313ad 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -838,10 +838,17 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq) static void sd_unprep_fn(struct request_queue *q, struct request *rq) { + struct scsi_cmnd *SCpnt = rq->special; + if (rq->cmd_flags & REQ_DISCARD) { free_page((unsigned long)rq->buffer); rq->buffer = NULL; } + if (SCpnt->cmnd != rq->cmd) { + mempool_free(SCpnt->cmnd, sd_cdb_pool); + SCpnt->cmnd = NULL; + SCpnt->cmd_len = 0; + } } /** @@ -1720,21 +1727,6 @@ static int sd_done(struct scsi_cmnd *SCpnt) if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt)) sd_dif_complete(SCpnt, good_bytes); - if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type) - == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd) { - - /* We have to print a failed command here as the - * extended CDB gets freed before scsi_io_completion() - * is called. - */ - if (result) - scsi_print_command(SCpnt); - - mempool_free(SCpnt->cmnd, sd_cdb_pool); - SCpnt->cmnd = NULL; - SCpnt->cmd_len = 0; - } - return good_bytes; } -- cgit v1.2.3