On Sun, 2014-03-02 at 19:51 +0200, Sagi Grimberg wrote:
> When copying between device and command protection scatters
> we must take into account that device scatters might be offset
> and we must not copy outside scatter range. Thus for each cmd prot
> scatter we must take the min between cmd prot scatter, dev prot
> scatter, and what's left (and loop in case we havn't copied enough
> from/to cmd prot scatter).
>
> This issue was found using iSER T10-PI offload over rd_mcp (wasn't
> discovered with fileio since file_dev prot sglists are never offset).
>
> Debug example (single t_prot_sg of len 2048):
> kernel: sbc_dif_copy_prot: se_cmd=ffff880380aaf970, left=2048, sg_off=3072, sg->length=4096, len=2048
> kernel: isert: se_cmd=ffff880380aaf970 PI error found type 0 at sector 0x2600 expected 0x0 vs actual 0x725f, lba=0x2580
>
> Instead of copying 2048 bytes from offset 3072 (copying junk
> outside sg limit 4096), we must copy 1024 bytes and continue to
> next sg until we complete cmd prot scatter (1024 from next sg
> in this case).
>
> Same example:
> kernel: sbc_dif_copy_prot: se_cmd=ffff8803ea6aba28, left=2048, offset=3072, sg->length=4096, len=1024
> kernel: sg = sg_next(sg)
> kernel: sbc_dif_copy_prot: se_cmd=ffff8803ea6aba28, left=1024, offset=0, sg->length=4096, len=1024
>
> Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> ---
A few comments below..
> drivers/target/target_core_sbc.c | 37 +++++++++++++++++++++----------------
> 1 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 0d1476a..ab006d2 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -1137,25 +1137,30 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
> left = sectors * dev->prot_length;
>
> for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
> -
> - len = min(psg->length, left);
> - if (offset >= sg->length) {
> - sg = sg_next(sg);
> - offset = 0;
> - }
> + unsigned int psg_len, copied = 0;
>
> paddr = kmap_atomic(sg_page(psg)) + psg->offset;
> - addr = kmap_atomic(sg_page(sg)) + sg->offset + offset;
> -
> - if (read)
> - memcpy(paddr, addr, len);
> - else
> - memcpy(addr, paddr, len);
> -
> - left -= len;
> - offset += len;
> + psg_len = min(left, psg->length - psg->offset);
So it's wrong to consider psg->offset in the length calculation here..
The psg->length should already be taking into account any non zero
psg->offset, without having to do a recalculation.
So, it's perfectly fine to expect:
sg->length = 1024
sg->offset = 3072
which can result in a bogus value with the current patch.
Eg, following SGL would be considered bogus:
sg->length = 4096
sg->offset = 3072
> + while (copied < psg_len) {
> + len = min(psg_len, sg->length - offset);
> + addr = kmap_atomic(sg_page(sg)) + sg->offset + offset;
> +
> + if (read)
> + memcpy(paddr, addr, len);
> + else
> + memcpy(addr, paddr, len);
> +
AFAICT paddr's location is incorrect on subsequent loops here, as it's
not taking the already copied byte offset into account..
I think this should be:
if (read)
memcpy(paddr + copied, addr, len);
else
memcpy(addr, paddr + copied, len);
> + left -= len;
> + offset += len;
> + copied += len;
> +
> + if (offset >= sg->length - sg->offset) {
Same issue here wrt to sg->offset as above.
--nab
> + sg = sg_next(sg);
> + offset = 0;
> + }
> + kunmap_atomic(addr);
> + }
> kunmap_atomic(paddr);
> - kunmap_atomic(addr);
> }
> }
>
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
