linux-kernel - Re: [PATCH v6 2/4] Add support for SCT Write Same

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: <CAGnHSEnZe0iipr2hO=QDSq6s7+5JoevXvPNes=WP7gwToFzHsw@mail.gmail.com>
Date:   Tue, 23 Aug 2016 04:14:32 +0800
From:   Tom Yan <tom.ty89@...il.com>
To:     Shaun Tancheff <shaun.tancheff@...gate.com>
Cc:     Shaun Tancheff <shaun@...cheff.com>, linux-ide@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>, Tejun Heo <tj@...nel.org>,
        Christoph Hellwig <hch@....de>,
        "Martin K . Petersen" <martin.petersen@...cle.com>,
        Damien Le Moal <damien.lemoal@...t.com>,
        Hannes Reinecke <hare@...e.de>,
        Josh Bingaman <josh.bingaman@...gate.com>,
        Hannes Reinecke <hare@...e.com>
Subject: Re: [PATCH v6 2/4] Add support for SCT Write Same

On 23 August 2016 at 03:43, Shaun Tancheff <shaun.tancheff@...gate.com> wrote:
>>> +       if (unmap) {
>>> +               /* If trim is not enabled the cmd is invalid. */
>>> +               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
>>> +                   !ata_id_has_trim(dev->id)) {
>>> +                       fp = 1;
>>> +                       bp = 3;
>>> +                       goto invalid_fld;
>>> +               }
>>> +               /* If the request is too large the cmd is invalid */
>>> +               if (n_block > 0xffff * trmax) {
>>> +                       fp = 2;
>>> +                       goto invalid_fld;
>>> +               }
>>
>> This response should be generally applied to the Write Same (16)
>> translation, since it is required by SBC,
>>
>>> +       } else {
>>> +               /* If write same is not available the cmd is invalid */
>>> +               if (!ata_id_sct_write_same(dev->id)) {
>>> +                       fp = 1;
>>> +                       bp = 3;
>>> +                       goto invalid_fld;
>>> +               }
>>
>> therefore, you should add an n_block check here as well, if you are
>> going to advertise an Maximum Write Same Length even when the device
>> supports only SCT Write Same but not TRIM. Most likely you would want
>> to simply move the existing check one-level up (if the same limit is
>> advertised no matter TRIM is supported not or not).
>
> Why would we enforce upper level limits on something that doesn't
> have any?

If we advertise a limit in our SATL, it makes sense that we should
make sure the behaviour is consistent when we issue a write same
through the block layer / ioctl and when we issue a SCSI Write Same
command directly (e.g. with sg_write_same). IMHO that's pretty much
why SBC would mandate such behaviour as well.

>
> If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
> wipe a whole disk it should be free to do so.
>

That's why I said, "if you are going to advertise an Maximum Write Same Length".