2008-08-18 22:16:21

by Anthony Liguori

[permalink] [raw]
Subject: [PATCH] virtio_balloon: fix towards_target when deflating balloon

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)


2008-08-19 00:43:21

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] virtio_balloon: fix towards_target when deflating balloon



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

2008-08-19 01:10:40

by Anthony Liguori

[permalink] [raw]
Subject: Re: [PATCH] virtio_balloon: fix towards_target when deflating balloon

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)

2008-08-19 03:08:17

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH] virtio_balloon: fix towards_target when deflating balloon

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.

2008-08-19 04:18:19

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] virtio_balloon: fix towards_target when deflating balloon



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