[Qemu-devel] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)

[Qemu-devel] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)

[Eric Blake]

When using bit-wise operations that exploit the power-of-two
nature of the second argument of ROUND_UP(), we still need to
ensure that the mask is as wide as the first argument (done
by using addition of 0 to force proper arithmetic promotion).
Unpatched, ROUND_UP(2ULL*1024*1024*1024*1024, 512) produces 0,
instead of the intended 2TiB.

Broken since its introduction in commit 292c8e50 (v1.5.0).

CC: qemu-trivial@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>

---
I did not audit to see how many potential users of ROUND_UP
are actually passing in different sized types where the first
argument can be larger than UINT32_MAX; I stumbled across the
problem when iotests 190 started failing on a patch where I
added a new use.  We can either be conservative and put this
on qemu-stable no matter what, or go through the effort of an
audit to see what might be broken (many callers in the block
layer, but not just there).
---
 include/qemu/osdep.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 6855b94bbf..7a3000efc5 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -189,13 +189,13 @@ extern int daemon(int, int);

 /* Round number up to multiple. Requires that d be a power of 2 (see
  * QEMU_ALIGN_UP for a safer but slower version on arbitrary
- * numbers) */
+ * numbers); works even if d is a smaller type than n.  */
 #ifndef ROUND_UP
-#define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
+#define ROUND_UP(n, d) (((n) + (d) - 1) & -((n) - (n) + (d)))
 #endif

 #ifndef DIV_ROUND_UP
-#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
 #endif

 /*
-- 
2.13.5

Re: [Qemu-devel] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)

[Paolo Bonzini]

On 13/09/2017 23:03, Eric Blake wrote:
> When using bit-wise operations that exploit the power-of-two
> nature of the second argument of ROUND_UP(), we still need to
> ensure that the mask is as wide as the first argument (done
> by using addition of 0 to force proper arithmetic promotion).
> Unpatched, ROUND_UP(2ULL*1024*1024*1024*1024, 512) produces 0,
> instead of the intended 2TiB.
> 
> Broken since its introduction in commit 292c8e50 (v1.5.0).
> 
> CC: qemu-trivial@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> I did not audit to see how many potential users of ROUND_UP
> are actually passing in different sized types where the first
> argument can be larger than UINT32_MAX; I stumbled across the
> problem when iotests 190 started failing on a patch where I
> added a new use.  We can either be conservative and put this
> on qemu-stable no matter what, or go through the effort of an
> audit to see what might be broken (many callers in the block
> layer, but not just there).
> ---
>  include/qemu/osdep.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94bbf..7a3000efc5 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -189,13 +189,13 @@ extern int daemon(int, int);
> 
>  /* Round number up to multiple. Requires that d be a power of 2 (see
>   * QEMU_ALIGN_UP for a safer but slower version on arbitrary
> - * numbers) */
> + * numbers); works even if d is a smaller type than n.  */
>  #ifndef ROUND_UP
> -#define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
> +#define ROUND_UP(n, d) (((n) + (d) - 1) & -((n) - (n) + (d)))
>  #endif
> 
>  #ifndef DIV_ROUND_UP
> -#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>  #endif
> 
>  /*
> 


Cc: qemu-stable@nongnu.org

Paolo

Re: [Qemu-devel] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)

[Laszlo Ersek]

On 09/13/17 23:03, Eric Blake wrote:
> When using bit-wise operations that exploit the power-of-two
> nature of the second argument of ROUND_UP(), we still need to
> ensure that the mask is as wide as the first argument (done
> by using addition of 0 to force proper arithmetic promotion).
> Unpatched, ROUND_UP(2ULL*1024*1024*1024*1024, 512) produces 0,
> instead of the intended 2TiB.
> 
> Broken since its introduction in commit 292c8e50 (v1.5.0).
> 
> CC: qemu-trivial@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> I did not audit to see how many potential users of ROUND_UP
> are actually passing in different sized types where the first
> argument can be larger than UINT32_MAX; I stumbled across the
> problem when iotests 190 started failing on a patch where I
> added a new use.  We can either be conservative and put this
> on qemu-stable no matter what, or go through the effort of an
> audit to see what might be broken (many callers in the block
> layer, but not just there).
> ---
>  include/qemu/osdep.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94bbf..7a3000efc5 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -189,13 +189,13 @@ extern int daemon(int, int);
> 
>  /* Round number up to multiple. Requires that d be a power of 2 (see
>   * QEMU_ALIGN_UP for a safer but slower version on arbitrary
> - * numbers) */
> + * numbers); works even if d is a smaller type than n.  */
>  #ifndef ROUND_UP
> -#define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
> +#define ROUND_UP(n, d) (((n) + (d) - 1) & -((n) - (n) + (d)))
>  #endif

Another way to widen the mask as necessary would be:

  (((n) + (d) - 1) & -(0 ? (n) : (d)))

>From the C standard,

6.5.15 Conditional operator
5 If both the second and third operands have arithmetic type, the result
  type that would be determined by the usual arithmetic conversions,
  were they applied to those two operands, is the type of the result.
  [...]

Using the conditional operator is *perhaps* better than addition and
subtraction, because it would keep the previous trait of ROUND_UP that
"n" is evaluated only once:

6.5.15 Conditional operator
4 The first operand is evaluated; there is a sequence point after its
  evaluation. The second operand is evaluated only if the first compares
  unequal to 0; the third operand is evaluated only if the first
  compares equal to 0; the result is the value of the second or third
  operand (whichever is evaluated), converted to the type described
  below. [...]

Although, I admit that invoking ROUND_UP with any side effects in the
arguments would be crazy to begin with...

> 
>  #ifndef DIV_ROUND_UP
> -#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>  #endif

This looks like an independent whitespace fix; should it be in this patch?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On 09/14/2017 03:44 AM, Laszlo Ersek wrote:
> On 09/13/17 23:03, Eric Blake wrote:
>> When using bit-wise operations that exploit the power-of-two
>> nature of the second argument of ROUND_UP(), we still need to
>> ensure that the mask is as wide as the first argument (done
>> by using addition of 0 to force proper arithmetic promotion).
>> Unpatched, ROUND_UP(2ULL*1024*1024*1024*1024, 512) produces 0,
>> instead of the intended 2TiB.
>>
>> Broken since its introduction in commit 292c8e50 (v1.5.0).
>>
>> CC: qemu-trivial@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> +#define ROUND_UP(n, d) (((n) + (d) - 1) & -((n) - (n) + (d)))
>>  #endif
> 
> Another way to widen the mask as necessary would be:
> 
>   (((n) + (d) - 1) & -(0 ? (n) : (d)))

Oh, I like that even better!


>>  #ifndef DIV_ROUND_UP
>> -#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
>> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>>  #endif
> 
> This looks like an independent whitespace fix; should it be in this patch?

checkpatch complained about the pre-patch spacing in ROUND_UP, and
DIV_ROUND_UP had the same issue 2 lines later.  But you're right that
it's not strictly necessary (or that I should at least call it out in
the commit message).

v2 coming up.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]