linux-kernel - Re: [PATCH] Do not silently discard WRITE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <543E1407.7020509@vmware.com>
Date:	Tue, 14 Oct 2014 23:28:23 -0700
From:	Petr Vandrovec <petr@...are.com>
To:	"Martin K. Petersen" <martin.petersen@...cle.com>
CC:	Jens Axboe <axboe@...nel.dk>,
	Arvind Kumar <arvindkumar@...are.com>,
	Chris J Arges <chris.j.arges@...onical.com>,
	Christoph Hellwig <hch@....de>, stable@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Do not silently discard WRITE_SAME requests

On 10/14/2014 5:57 PM, Martin K. Petersen wrote:
>>>>>> "Petr" == Petr Vandrovec <petr@...are.com> writes:
>
> Petr,
>
> Petr> Logic (from 2011, commit 8af1954d172a46a63e5e79dae523a6d74715e458)
> Petr> says that EOPNOTSUPP is returned when DISCARD request failed, as
> Petr> discarding is optional, and failures can be safely ignored.  That
> Petr> is definitely not true for WRITE_SAME failures, and so unsupported
> Petr> WRITE_SAME should return different error code than unsupported
> Petr> DISCARD.
>
> I agree. With Lukas' change the bio batch error handling is too
> permissive for the write same case. It also made the regular zeroout
> error handling case a bit fishy but it is unlikely that we'd get
> EOPNOTSUPP on a regular write.
>
> So for the block portion:
>
> Acked-by: Martin K. Petersen <martin.petersen@...cle.com>
> Cc: stable@...r.kernel.org # 3.7+
>
> Petr> revert of blacklisting VMware's LSI (if anything, blacklist should
> Petr> be for current firmware version of 'VMware Virtual SCSI Disk', as
> Petr> f.e. passed-through (RDM) SCSI disks do support WRITE_SAME under
> Petr> VMware) -- see attached updated diff.
>
> If you have a better heuristic then let's by all means use it. Please
> adjust the pattern matching strings to match the actual values returned
> by the target.
>
>
> SCSI: Only blacklist WRITE SAME for VMware virtual disks
>
> Commit 4089b71cc820 blacklisted WRITE SAME for all VMware disks.
> However, the WRITE SAME commands are supported for passthrough
> disks. Change the heuristic so we only blacklist virtual disks.

Hi,
   is there any reason to do blacklisting?  Why not let first request 
fail, and switch to non-write-same code path once that happens?

If you want to go ahead with blacklisting, then entries should be:

{ "VMware", "Virtual disk", "1.0", BLIST_NO_WRITE_SAME },   //ESX
{ "VMware,", "VMware Virtual S", "1.0", BLIST_NO_WRITE_SAME }, //WS,Fusion

(yes, on Workstation and Fusion there is really comma in the vendor name 
and VMware is repeated in the device ID - first is 'VMware, Inc.' 
truncated to 8 characters, and second is 'VMware Virtual SCSI Hard 
Drive' truncated to 16; we got revision right...)
				Thanks,
					Petr


> Signed-off-by: Martin K. Petersen <martin.petersen@...cle.com>
> Reported-by: Petr Vandrovec <petr@...are.com>
>
> diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
> index 613231c16194..787933d43d32 100644
> --- a/drivers/message/fusion/mptspi.c
> +++ b/drivers/message/fusion/mptspi.c
> @@ -1419,11 +1419,6 @@ mptspi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		goto out_mptspi_probe;
>           }
>
> -	/* VMWare emulation doesn't properly implement WRITE_SAME
> -	 */
> -	if (pdev->subsystem_vendor == 0x15AD)
> -		sh->no_write_same = 1;
> -
>   	spin_lock_irqsave(&ioc->FreeQlock, flags);
>
>   	/* Attach the SCSI Host to the IOC structure
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index 49014a143c6a..402aebfb97dd 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -249,6 +249,7 @@ static struct {
>   	{"TOSHIBA", "CD-ROM", NULL, BLIST_ISROM},
>   	{"Traxdata", "CDR4120", NULL, BLIST_NOLUN},	/* locks up */
>   	{"USB2.0", "SMARTMEDIA/XD", NULL, BLIST_FORCELUN | BLIST_INQUIRY_36},
> +	{"VMware", "Virtual SCSI Disk", NULL, BLIST_NO_WRITE_SAME},
>   	{"WangDAT", "Model 2600", "01.7", BLIST_SELECT_NO_ATN},
>   	{"WangDAT", "Model 3200", "02.2", BLIST_SELECT_NO_ATN},
>   	{"WangDAT", "Model 1300", "02.4", BLIST_SELECT_NO_ATN},
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ba3f1e8d0d57..c57daffe57af 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -955,6 +955,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>   	if (*bflags & BLIST_NO_DIF)
>   		sdev->no_dif = 1;
>
> +	if (*bflags & BLIST_NO_WRITE_SAME)
> +		sdev->no_write_same = 1;
> +
>   	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>
>   	if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 183eaab7c380..1a24efb4b1d6 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -36,5 +36,6 @@
>   					     for sequential scan */
>   #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
>   #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
> +#define BLIST_NO_WRITE_SAME	0x40000000 /* don't try to issue WRITE SAME */
>
>   #endif
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/