Re: [Xen-devel] [PATCH v2] x86/HVM: avoid pointer wraparound in bufioreq handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.07.15 at 18:18, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 18 Jun 2015, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -921,7 +921,7 @@ static void hvm_ioreq_server_disable(str
>>  
>>  static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain 
>> *d,
>>                                   domid_t domid, bool_t is_default,
>> -                                 bool_t handle_bufioreq, ioservid_t id)
>> +                                 int bufioreq_handling, ioservid_t id)
> 
> uint8_t?

Why? I'm generally against using fixed width types when you don't
really need them. And using uint_least8_t or uint_fast8_t is neither
an opton, nor would it make the code look reasonable. Plain int is
just fine here.

>> @@ -2568,17 +2575,29 @@ int hvm_buffered_io_send(ioreq_t *p)
>>          return 0;
>>      }
>>  
>> -    pg->buf_ioreq[pg->write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
>> +    pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
>>  
>>      if ( qw )
>>      {
>>          bp.data = p->data >> 32;
>> -        pg->buf_ioreq[(pg->write_pointer+1) % IOREQ_BUFFER_SLOT_NUM] = bp;
>> +        pg->buf_ioreq[(pg->ptrs.write_pointer+1) % IOREQ_BUFFER_SLOT_NUM] = 
>> bp;
>>      }
>>  
>>      /* Make the ioreq_t visible /before/ write_pointer. */
>>      wmb();
>> -    pg->write_pointer += qw ? 2 : 1;
>> +    pg->ptrs.write_pointer += qw ? 2 : 1;
>> +
>> +    /* Canonicalize read/write pointers to prevent their overflow. */
>> +    while ( s->bufioreq_atomic && qw++ < IOREQ_BUFFER_SLOT_NUM &&
>> +            pg->ptrs.read_pointer >= IOREQ_BUFFER_SLOT_NUM )
>> +    {
>> +        union bufioreq_pointers old = pg->ptrs, new;
>> +        unsigned int n = old.read_pointer / IOREQ_BUFFER_SLOT_NUM;
>> +
>> +        new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>> +        new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>> +        cmpxchg(&pg->ptrs.full, old.full, new.full);
>> +    }
> 
> This is not safe: if the reader increments read_pointer (atomically)
> after you read old.read_pointer and before cmpxchg, the increment is
> lost.

But that's what I use cmpxchg() for - if the value changes between
the read and the cmpxchg, the latter will indicate so (for the purpose
here simply by not updating the memory location), and the loop will go
through another iteration.

> I think you need to remove the cmpxchg and subtract a multiple of
> IOREQ_BUFFER_SLOT_NUM from read_pointer atomically here. Unfortunately
> if we do that, we cannot update both write_pointer and read_pointer
> together anymore. But if we decrement write_pointer atomically before
> read_pointer, I think we should be fine if we appropriately fix the
> reader side too:
> 
> QEMU:
>   atomic_read(&read_pointer);
>   read write_pointer;
>   xen_rmb();
>   while (read_pointer < write_pointer) {

That would cause this comparison to possibly evaluate to "false", and
I don't think leaving this loop early can bring any good.

Jan

> 
>       [work]
> 
>       atomic_add(&read_pointer, stuff);
>       read write_pointer;
>       xen_rmb();
>   }
> 
> 
> Xen:
>   [work]
>   increment write_pointer;
>   xen_wmb();
> 
>   if (read_pointer >= IOREQ_BUFFER_SLOT_NUM) {
>       write_pointer -= IOREQ_BUFFER_SLOT_NUM;
>       xen_wmb();
>       atomic_sub(read_pointer, IOREQ_BUFFER_SLOT_NUM);
>   }




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel