Message ID | 20170913210343.19078-1-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Series | osdep: Fix ROUND_UP(64-bit, 32-bit) | expand |
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
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
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.
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 /*
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(-)