Re: [PATCH] virtio-balloon: do not attempt to release more than available pages

[Marcelo Tosatti <marcelo@kvack.org>]

On Wed, Mar 05, 2008 at 06:59:10PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
> >Handle the case where the balloon target is larger than total ram size.
> >
> >BUG: unable to handle kernel paging request at 0000000000100100
> >IP: [<ffffffff881970f9>] :virtio_balloon:leak__balloon+0x2e/0xbe
> >
> >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >Index: virtio/virtio_balloon.c
> >===================================================================
> >--- a/drivers/virtio/virtio_balloon.c
> >+++ b/drivers/virtio/virtio_balloon.c
> >@@ -122,10 +122,21 @@ static void release_pages_by_pfn(const u
> > 	}
> > }
> > 
> >+static void update_target_size(struct virtio_balloon *vb)
> >+{
> >+	__le32 num_pages = cpu_to_le32(vb->num_pages);
> >+
> >+	vb->vdev->config->set(vb->vdev,
> >+			      offsetof(struct virtio_balloon_config, 
> >num_pages),
> >+			      &num_pages, sizeof(num_pages));
> >+}
> >  
> 
> The target is host-owned; moreover the problem may be temporary, but 
> you've changed the target permanently.
> 
> Suggest sending the host a message (like the page list) indicating it 
> couldn't allocate any more.
> 
> Also, we may have driven the guest close to oom with this.  We need to 
> notify the host when the guest gets into a low-memory cannot swap condition.

I guess the description was not clear, you understood the opposite.

The problem is when the target for total guest pages (not balloon target
size) is set to be larger than the amount of total pages the guest has
booted with. What happens then is that the driver tries to release pages
from the balloon, without checking if there are any:

static void leak_balloon(struct virtio_balloon *vb, size_t num)
{
        struct page *page;

	/* We can only do one array worth at a time. */
	num = min(num, ARRAY_SIZE(vb->pfns));

        for (vb->num_pfns = 0; vb->num_pfns < num; vb->num_pfns++) {
                page = list_first_entry(&vb->pages, struct page, lru);
                list_del(&page->lru);
                vb->pfns[vb->num_pfns] = page_to_pfn(page);
                vb->num_pages--;
        }

vp->pages is empty here.

So the patch checks for the availability of ballooned pages before
attempting to release any, and sets num_pages to match that. 

The host should not allow that to condition to happen, but its still
fragile code in the guest driver.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/