On 23 August 2016 at 08:37, Tom Yan <tom.ty89@xxxxxxxxx> wrote:
> On 23 August 2016 at 07:30, Shaun Tancheff <shaun@xxxxxxxxxxxx> wrote:
>>
>> But the "humanized" limit is the one you just added and proceeded to
>> change ata_set_lba_range_entries(). You changed from a buffer size
>> to use "num" instead and now you want to remove the protection
>> entirely?
>
> I am not sure about what you are talking about here. What I did is
> just to avoid setting an (inappropriate and unnecessary) hard-coded
> limit (with the original while-condition) for i (the unaligned buffer
> size) in this general (in the sense that TRIM payload is _not always_
> of one 512-byte block, even if we may want to forever practice that in
> our kernel) function.
>
> If we really want/need to avoid hitting some real buffer limit (e.g.
> maximum length of scatter/gather list?), then we should in some way
> check n_block against that. If it is too large we then return
> used_bytes = 0 (optionally with some follow-up to add a response to
> such return value or so).
>
> We advertise 4194240 as Maximum Write Same Length so that the
> SCSI/block layer will know how to split the discard request, and then
> we make sure that the SATL reject SCSI commands (that is not issued
> through the discard / write same ioctl but with SG_IO /
> sg_write_same). That's all we really need to do, end of story.
>
>>
>> Why not just change to put this in front of ata_set_lba_range_entries()
>>
>> if (n_block > 65535 * ATA_MAX_TRIM_RNUM) {
>> fp = 2;
>> goto invalid_fld;
>> }
>
> Well you can change the if-else clause to that. Apparently you've been
> doing that in your patch series. But that has nothing to do with what
> I am fixing here. Neither does it affect the actual behavior.
>
>>
>> And then restore ata_set_lba_range_entries() to how it looked
>> before you changed it in commit:
>>
>> 2983860c7 (libata-scsi: avoid repeated calculation of number of TRIM ranges)
>
> I don't see how it is relevant at all. Though I should have introduced
> _this_ patch before that to save some work. Unfortunately I wasn't
> aware how ugly ata_set_lba_range_entries was.
>
>>
>> Then you can have ata_set_lba_range_entries() take the buffer size ...
>> something like the following would be fine:
>>
>> size = ata_set_lba_range_entries(buf, scmd->device->sector_size,
>> block, n_block);
>>
>> Now things are happily protected against both exceeding the b0 limit(s) and
>> overflowing the sglist buffer.
>
> We have no reason at all to bring the logical sector size in here.
> Only if in future ACS standards it is stated that payload block are
> counted in logical block size instead of statically 512, we would only
> need to make the now static "512" in ALIGN() in to 4096.
typo. I meant from static "512" to logical sector size, and I really
think we should do it ONLY if the later ACS changes its statement.
>
> For possible sglist buffer overflow, see what I mentioned above about
> checking n_block and return 0. We would not want to do it with the
> original while-way anyway. All it does would be to truncate larger
> request and partially complete it silently.
>
> Can you tell exactly how the size of the sglist buffer is limited?
> AFAIK it has nothing to do with logical sector size. I doubt that we
> will ever cause an overflow, given how conservative we have been on
> the size of TRIM payload. We would probably ignore the reported value
> once it is larger than 16 or so, if we would ever enable multi-block
> payload in the future.
>
> Realistically speaking, I sent this patch not really to get the
> function ready for multi-block payload (neither I have mentioned that
> in the commit message), but just to make the code proper and avoid
> silly effort (and confusion caused) you and I spent in our patches.
>
>>
>> --
>> Shaun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
