On Tue, Aug 23, 2016 at 5:17 AM, Tom Yan <tom.ty89@xxxxxxxxx> wrote: > 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 <hch@xxxxxx> > 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. > 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. > 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 is always sdp->sector_size > 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. However, the return value of > 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. > 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. > Though IMHO this is really NOT a reason that is strong enough to > prevent this patch from entering the repo first. > On 23 August 2016 at 09:36, Tom Yan <tom.ty89@xxxxxxxxx> wrote: >> On 23 August 2016 at 09:18, Shaun Tancheff <shaun.tancheff@xxxxxxxxxxx> wrote: >>> On Tue, Aug 23, 2016 at 3:37 AM, Tom Yan <tom.ty89@xxxxxxxxx> wrote: >>>> On 23 August 2016 at 07:30, Shaun Tancheff <shaun@xxxxxxxxxxxx> wrote: >>> >>>> 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. >> >>> >>> 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. >> >>> >>> -- >>> Shaun -- Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
