Re: [Qemu-stable] [Qemu-devel] [PATCH] virtio: fix vring->inuse recalc after migr

[Stefan Hajnoczi]

On Fri, Dec 16, 2016 at 05:43:29PM +0100, Halil Pasic wrote:
> 
> 
> On 12/16/2016 05:12 PM, Stefan Hajnoczi wrote:
> >> You are not the first one complaining, so the sentence is definitively
> >> bad. What disturbs me regarding your formulation is that we do not use
> >> uint16_t to represent neither the ring size nor inuse.
> >>
> >> How about "Since max ring size < UINT16_MAX it's safe to use modulo
> >> UINT16_MAX + 1 subtraction."?
> > That doesn't mention "representing the size of the ring" so it's
> > unclear what "safe" means.
> > 
> > Stefan
> > 
> 
> IMHO it is not about representation but about correct arithmetic.
> We introduce the cast, not because representing the ring size as
> int is necessarily an issue, but because we ended up with a wrong
> result. In my opinion how can 'inuse' be represented correctly and
> efficiently concerns the member of struct VirtQueue.

Fair enough, the type of VirtQueue.inuse doesn't need to be justified at
this point in the code.

> Here the important point is how conversions between signed unsigned
> integer types work in C.
> 
> """
> 6.3.1.3 Signed and unsigned integers
> 1 When a value with integer type is converted to another integer
> type other than _Bool, if  the value can be represented by the new
> type, it is unchanged.
> 2 Otherwise, if the new type is unsigned, the value is converted by
> repeatedly adding or  subtracting one more than the maximum value that
> can be represented in the new type until the value is in the range
> of the new type.
> """
> 
> That is we get mod UINT16_MAX + 1  subtraction which is what we
> need if we want to calculate the difference between the two counters
> under the assumption that the actual conceptual difference (that
> is if the counters where of type arbitrary natural) is less or equal that
> queue size which is less than UINT16_MAX.

-            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
-                                vdev->vq[i].used_idx;
+            vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx -
+                                vdev->vq[i].used_idx);

Looking at C99 I learnt the cause of the bug is the integer promotion
performed on the uint16_t subtraction operands.  Previously I was only
aware of integer promotion on varargs and function arguments where no
prototype is declared.

So the original statement behaves like:

vdev->vq[i].inuse = (int)vdev->vq[i].last_avail_idx -
                    (int)vdev->vq[i].used_idx;

This is the real gotcha for me.

If you feel it helps to explain the signed -> unsigned cast behavior,
feel free.  I don't think it's necessary.

Stefan

signature.asc
Description: PGP signature