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]

Date:	Sat, 11 Oct 2014 23:18:02 -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/11/2014 5:51 AM, Martin K. Petersen wrote:
>>>>>> "Petr" == Petr Vandrovec <petr@...are.com> writes:
>
> Petr> After investigating, problem seems to be in a way completion
> Petr> handler for WRITE_SAME handles EOPNOTSUPP error, causing
> Petr> all-but-first WRITE_SAME request on the LVM device to be silently
> Petr> ignored - command is never issued, but success is returned to
> Petr> higher layers.
>
> Commit 7eee4ae2dbb2 was meant to address this issue. Does it still
> happen with that in place?

Hi,
   that commit alleviates need for change to blk-lib.c.  But I believe 
that change to blk-core.c that changes return value from EOPNOTSUPP to 
EREMOTEIO is still necessary - unless I'm missing some locking 
somewhere, there is a race in blkdev_issue_write_same() wrt. updating 
max_write_same_sectors:

blkdev_issue_write_same() checks whether max_write_same_sectors is 
non-zero at the beginning, and if it is non-zero it proceeds with 
generating BIOs.  While it generates them, other thread seems to be able 
to complete previously issued write_same, find it is not supported, and 
clear max_write_same_sectors.  Which means that BIOs that are now being 
generated will fast-fail in blk-core.c with EOPNOTSUPP, and 
blkdev_issue_write_same() will then return success, rather than failure.

It is true that now WRITE_SAME is failing only if second WRITE_SAME is 
issued to the device while first ever issued WRITE_SAME on the device is 
being completed, but I see no reason why to not close this race.

Logic (from 2011, commit 8af1954d172a46a63e5e79dae523a6d74715e458) says 
that EOPNOTSUPP is returned when DISCARD request failed, as discarding 
is optional, and failures can be safely ignored.  That is definitely not 
true for WRITE_SAME failures, and so unsupported WRITE_SAME should 
return different error code than unsupported DISCARD.

Which is what patch does.  I've removed part that propagates disabling 
WRITE_SAME from the diff, keeping only EOPNOTSUPP => EREMOTEIO change, 
and revert of blacklisting VMware's LSI (if anything, blacklist should 
be for current firmware version of 'VMware Virtual SCSI Disk', as f.e. 
passed-through (RDM) SCSI disks do support WRITE_SAME under VMware) -- 
see attached updated diff.
					Petr Vandrovec

View attachment "0001-Do-not-silently-discard-WRITE_SAME-requests.patch" of type "text/plain" (2129 bytes)