Re: [Qemu-devel] [PATCH v3] scsi: pvscsi: avoid infinite loop while building SG list

[Dmitry Fleytman]

> On 13 Sep 2016, at 14:20 PM, P J P <address@hidden> wrote:
> 
>  Hello Dmitry,
> 
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | Better put here:
> | 
> | if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
> |     return;
> | }
> |
> | And ditch additional conditions from while clauses.
> | This way you’ll have less compare operations and avoid
> | adding the same element twice when leaving because of too long SG list.
> 
>  Okay.
> 
> | Also this is not a good idea to send truncated requests
> | when SG list becomes too long. I’d prefer to return error
> | code from pvscsi_convert_sglist() and handle it as appropriate.
> 
>  This could be a separate patch. Should pvscsi_build_sglist() destroy the 
> sglist on error? 

It should be in the same patch because otherwise a broken logic will be 
introduced.

pvscsi_build_sglist() must perform all rollback actions required to handle the 
error cleanly.
Please also note that pvscsi_build_sglist() caller(s) should be aware of SG 
list creation failure
and handle this error scenario accordingly.

> 
> @@ -656,7 +660,9 @@ pvscsi_build_sglist(PVSCSIState *s, PVSCSIRequest *r)
> 
>     pci_dma_sglist_init(&r->sgl, d, 1);
>     if (r->req.flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
> -        pvscsi_convert_sglist(r);
> +        if (pvscsi_convert_sglist(r) < 0) {
> +            qemu_sglist_destroy(&r->sgl);
> +        }
>     } else {
>         qemu_sglist_add(&r->sgl, r->req.dataAddr, r->req.dataLen);
>     }
> 
> I'll send both patches together once you confirm.
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F