Both v and vb->num_pages are u32 and unsigned int respectively. If v is less
than vb->num_pages (and it is, when deflating the balloon), the result is a
very large 32-bit number. Since we're returning a s64, instead of getting the
same negative number we desire, we get a very large positive number.
This handles the case where v < vb->num_pages and ensures we get a small,
negative, s64 as the result.
Rusty: please push this for 2.6.27-rc4. It's probably appropriate for the
stable tree too as it will cause an unexpected OOM when ballooning.
Signed-off-by: Anthony Liguori <[email protected]>
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index bfef604..bd3c384 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -158,7 +158,10 @@ static inline s64 towards_target(struct virtio_balloon *vb)
vb->vdev->config->get(vb->vdev,
offsetof(struct virtio_balloon_config, num_pages),
&v, sizeof(v));
- return v - vb->num_pages;
+ if (v < vb->num_pages)
+ return -(s64)(vb->num_pages - v);
+ else
+ return v - vb->num_pages;
}
static void update_balloon_size(struct virtio_balloon *vb)
On Mon, 18 Aug 2008, Anthony Liguori wrote: <
>
> This handles the case where v < vb->num_pages and ensures we get a
> small, negative, s64 as the result.
That's just horrible code.
Maybe the compiler notices that you're doing something stupid, but
basically, please don't do this.
> - return v - vb->num_pages;
> + if (v < vb->num_pages)
> + return -(s64)(vb->num_pages - v);
> + else
> + return v - vb->num_pages;
What's wrong with just doing
return (s64)v - vb->num_pages;
instead?
Casting 'v' to s64 guarantees that the subtraction will eb done in 64
bits, and the compiler can just generate the trivial non-conditional code.
Linus
Linus Torvalds wrote:
> What's wrong with just doing
>
> return (s64)v - vb->num_pages;
>
> instead?
>
Nothing. It works just fine. However, I implemented it more verbosely
because this is the second time we've "fixed" this problem. See
commit bdc1681cdf1ab6a65fa935a2b3f8fc63b20c54ea
Author: Rusty Russell <[email protected]>
Date: Mon Mar 17 22:58:15 2008 -0500
virtio: handle > 2 billion page balloon targets
So I thought I'd rely a little less on the subtleties of promotion and
make things a bit more clear. However, I don't feel that strongly about
it so here you go.
Both v and vb->num_pages are u32 and unsigned int respectively. If v is
less
than vb->num_pages (and it is, when deflating the balloon), the result is a
very large 32-bit number. Since we're returning a s64, instead of
getting the
small negative number we desire, we get a very large positive number.
This patch explicitly casts v to a s64 in which will cause the whole
expression
to be promoted resulting in the proper results.
Rusty: please push this for 2.6.27-rc4. It's probably appropriate for the
stable tree too as it will cause an unexpected OOM when ballooning.
Signed-off-by: Anthony Liguori <[email protected]>
diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
index bfef604..62eab43 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -158,7 +158,7 @@ static inline s64 towards_target(struct
virtio_balloon *vb)
vb->vdev->config->get(vb->vdev,
offsetof(struct virtio_balloon_config, num_pages),
&v, sizeof(v));
- return v - vb->num_pages;
+ return (s64)v - vb->num_pages;
}
static void update_balloon_size(struct virtio_balloon *vb)
On Tuesday 19 August 2008 08:15:31 Anthony Liguori wrote:
> - return v - vb->num_pages;
> + if (v < vb->num_pages)
> + return -(s64)(vb->num_pages - v);
> + else
> + return v - vb->num_pages;
With all due respect, WTF?
Did you mean:
return (s64)v - vb->num_pages;
I'm really amazed this bug got this far though...
Rusty.
On Mon, 18 Aug 2008, Anthony Liguori wrote:
>
> Nothing. It works just fine. However, I implemented it more verbosely
> because this is the second time we've "fixed" this problem. See
>
> commit bdc1681cdf1ab6a65fa935a2b3f8fc63b20c54ea
> Author: Rusty Russell <[email protected]>
> Date: Mon Mar 17 22:58:15 2008 -0500
>
> virtio: handle > 2 billion page balloon targets
Well, we could perhaps add a sparse warning that makes noise when a
unsigned subtraction is cast to a wider signed field. I dunno if it would
catch anything interesting, or just cause a ton of irritating noise.
> So I thought I'd rely a little less on the subtleties of promotion and make
> things a bit more clear. However, I don't feel that strongly about it so here
> you go.
Ugly and inefficient is not acceptable, even for these kinds of reasons.
So yes, the simpler version is much better.
Linus