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

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
> 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.
>
>   

Ah, I see.  We could simply ignore it, or adjust the target as you did.  
Not sure what is better.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
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/