Re: [PATCH] ata: do not hard code limit in ata_set_lba_range_entries()

On 23 August 2016 at 10:45, Shaun Tancheff [off-list ref] wrote:

On Tue, Aug 23, 2016 at 5:17 AM, Tom Yan [off-list ref] wrote:

quoted

Wait a minute. I think you missed or misunderstood something when you
listen to someone's opinion in that we should switch to sglist buffer.

No, I think I can trust Christoph Hellwig [off-list ref]

I didn't say that you cannot trust him, but I just wonder you might
have misinterpreted what he said. I haven't really follow your patch
series earlier though.

quoted

I think the danger people referred to is exactly what is revealed when
the ugly code is removed in this commit (it doesn't mean that the code
should be kept though).

The original buffer appears to be open:
buf = page_address(sg_page(scsi_sglist(scmd)));

Which is unsafe.

Yup, but the while loop is not the right way to make it safe. It
probably prevent any overflow, but does not handle / response to the
request (n_block) properly.

It would only be practically unsafe if we would ever call it with a
request that needs a multi-block payload.

quoted

While the new buffer you adopted in in ata_format_sct_write_same() and
ata_format_dsm_trim_descr() is of fixed size:

Yes ... it is a temporary response buffer for simulated commands used to
copy data to and from the command sg_list so as not to hold irqs while
modifying the buffer.

quoted

buffer = ((void *)ata_scsi_rbuf);

sctpg = ((void *)ata_scsi_rbuf);

because:

#define ATA_SCSI_RBUF_SIZE      4096
...
static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];

So the sglist buffer is always 4096 bytes.

No. The sglist buffer attached to the write same / trim command

Don't you think it is in appropriate to use ata_scsi_rbuf here then?

is always sdp->sector_size

I can hardly see how sdp->sector_size is relevant to the sglist buffer
(especially the sglist here is facing to the ATA device).
Scatter/gather list could have multiple entries of a certain size
(that is not necessarily, or unlikely, logical sector size). Although
I don't really know how scatter/gather list works, it hardly seems
making any sense by saying that the sglist buffer is always logical
sector size.

AFAIK, for example, USB Attached SCSI driver does not limit the
maximum number of entries (max_segment) so the kernel fall back to
SG_MAX_SEGMENTS (2048), while it reports a max_segment_size of 4096 in
spite of the logical sector size with dma_get_max_seg_size().

For AHCI, it reports a max_segment of 168, while it does not seem to
report any max_segment_size so dma_get_max_seg_size() fallback to
65536 (though practically it seems 4096 will still be used in normal
writing).

quoted

And hence you can probably safely use ATA_SCSI_RBUF_SIZE as the buflen
param in the sg_copy_from_buffer() calls (at least in the case of
ata_format_sct_write_same()).

No. SCT Write Same has a fixed single 512 byte transfer.

Never mind on that. I have presumption that you should always fully
copy ata_scsi_rbuf to the sglist.

However, the return value of

quoted

ata_format_dsm_trim_descr() should still always be used_bytes since
that is needed by the ata taskfile construction.

So long as it does not exceed its sglist/sector_size buffer.

quoted

You may want to check (n_block / 65535 * 8 > ATA_SCSI_RBUF_SIZE). If
it is true, then perhaps we may want to return 0, and make the SATL
response with invalid CDB field if we catch that.

No that is not quite right you need to check if you are
overflowing either RBUF or sdp->sector_size.

Again, this does not make any sense. See above. Apparently you really
have no reason to use ata_scsi_rbuf.

quoted

Though IMHO this is really NOT a reason that is strong enough to
prevent this patch from entering the repo first.

quoted

On 23 August 2016 at 09:36, Tom Yan [off-list ref] wrote:

quoted

On 23 August 2016 at 09:18, Shaun Tancheff [off-list ref] wrote:

quoted

On Tue, Aug 23, 2016 at 3:37 AM, Tom Yan [off-list ref] wrote:

quoted

On 23 August 2016 at 07:30, Shaun Tancheff [off-list ref] wrote:

quoted

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).

Yes there is a real buffer limit, I can think of these two options:
1- Assume the setups from sd_setup_discard_cmnd() and/
   or sd_setup_write_same_cmnd() are providing an sglist of
   sdp->sector_size via scsi_init_io()

That sounds completely wrong. The scatter/gather list we are talking
about here has nothing to do with the SCSI or block layer anymore. The
SATL has _already_ parsed the SCSI Write Same (16) command and is
packing ranges/payload according to that in this stage. If there is
any limit it would probably the max_segment allowed by the host driver
(e.g. ahci).

It doesn't seem to make sense to me either that we would need to
prevent sglist overflow in such level. Doesn't that mean we would need
to do the same checking (specifically, as in hard coding checks in all
kinds of procedures) in every use of scatter/gather list? That doesn't
sound right at all.

quoted

2- Find (or write) a suitable sg_get_size(sgl, nents) to walk the
    sglist and calculate the available buffer size.

#2 sounds like more fun but I'm not sure it's what people would prefer to see.

No idea if such thing exists / makes sense at all.

quoted

--
Shaun

--
Shaun Tancheff