Re: [Qemu-devel] [PATCH] scsi: pvscsi: limit process IO loop to maximum page count

[Dmitry Fleytman]

> On 13 Sep 2016, at 13:48 PM, P J P <address@hidden> wrote:
> 
>  Hello Dmitry,
> 
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | > On 13 Sep 2016, at 10:00 AM, P J P <address@hidden> wrote:
> | > 
> | > +-- On Wed, 7 Sep 2016, P J P wrote --+
> | > | From: Prasad J Pandit <address@hidden>
> | > | 
> | > | Vmware Paravirtual SCSI emulator while processing IO requests
> | > | could run into an infinite loop if 'pvscsi_ring_pop_req_descr'
> | > | always returned positive value. Limit IO loop to the maximum
> | 
> | Do you see any specific scenario why this might happen?
> 
>  A guest user could set the 'ready_ptr' and 'PVSCSIRingInfo *mgr' parameter 
> in 'pvscsi_ring_pop_req_descr', such that it always returns true.

I see. The problematic code is if (ready_ptr != mgr->consumed_ptr) {…}

mgr->consumed_ptr is managed by device and not visible to the driver,
but ready_ptr is managed by driver and may be set to some “big” number.

In this case it may take a lot of iterations for consumed_ptr
to become equal to ready_ptr and additionally some requests will be send 
multiple times.

The most straightforward way to fix this issue will be to
check that ready_ptr - consumed_ptr is less than ring size.

> 
> | > | Reported-by: Li Qiang <address@hidden>
> | > |  pvscsi_process_io(PVSCSIState *s)
> | > |  {
> | > | +    int descr_pa_cnt = 0;
> | > |      PVSCSIRingReqDesc descr;
> | > |      hwaddr next_descr_pa;
> | > |  
> | > |      assert(s->rings_info_valid);
> | > | -    while ((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 
> 0) {
> | > | +    while (((next_descr_pa = pvscsi_ring_pop_req_descr(&s->rings)) != 
> 0)
> | > | +            && descr_pa_cnt++ < PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) {
> | 
> | Why do you limit number of processed descriptors by maximal number of pages 
> | in data exchange ring? What will happen to requests still waiting in the 
> | ring after this function exits?
> 
>  I limit it to maximum page count thinking that the descriptor value returned 
> by pvscsi_ring_pop_req_descr() is derived from the mgr->req_ring_pages_pa[] 
> array, which is of size 'PVSCSI_SETUP_RINGS_MAX_NUM_PAGES'. If 
> pvscsi_process_io() was to go into an infinite loop, it'd continue processing 
> the same set of req_ring_pages?

I think you’re mixing concepts of number of
pages in the ring and number of requests in the ring.

Each page contains (much) more than one request.

> 
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F