linux-kernel - Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 6 Mar 2013 17:52:28 +0000
From:	David Vrabel <david.vrabel@...rix.com>
To:	Daniel Kiper <daniel.kiper@...cle.com>
CC:	"carsten@...iers.de" <carsten@...iers.de>,
	"darren.s.shepherd@...il.com" <darren.s.shepherd@...il.com>,
	"james-xen@...gwall.me.uk" <james-xen@...gwall.me.uk>,
	"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>
Subject: Re: [PATCH 1/1] xen/balloon: Enforce various limits on target

On 06/03/13 16:47, Daniel Kiper wrote:
> On Wed, Mar 06, 2013 at 11:05:03AM +0000, David Vrabel wrote:
>> On 04/03/13 21:14, Daniel Kiper wrote:
>>> This patch enforces on target limit statically defined in Linux Kernel
>>> source and limit defined by hypervisor or host.
>>>
>>> Particularly this patch fixes bug which led to flood
>>> of dom0 kernel log with messages similar to:
>>>
>>> System RAM resource [mem 0x1b8000000-0x1bfffffff] cannot be added
>>> xen_balloon: reserve_additional_memory: add_memory() failed: -17
>>
>> I think this helps in some cases, but because
>> reserve_additional_memory() places the hotplugged memory after max_pfn
>> without checking if there is anything already there, there are still
>> ways it can repeatedly fail.
>>
>> e.g.,
>>
>> 1. If dom0 has had its maximum reservation limited initially (with the
>> dom0_mem option) /and/ the max reservation and target is subsequently
>> raised then the balloon driver will attempt to hotplug memory that
>> overlaps with E820_UNUSABLE regions in the e820 map and the hotplug will
>> fail.
>>
>> 2. If a domU has passed-through PCI devices max_pfn is before the PCI
>> memory window then the balloon driver will attempt to hotplug memory
>> over the PCI device BARs.
> 
> You are right. During work on this patch I discovered that but decided
> to enforce limits because it is more generic. Please look below why.
> However, I stated that it should be fixed too. I added it to my todo list.
> It requires a bit more work because I think new algorithm should cover
> many different cases. Probably add_memory() (it requires changes in
> mm/memory_hotplug.c) should be modified to look for range having
> sufficient size and not conflicting with others.

Ok, so we're agreed, this patch doesn't fix everything and that's fine.

>> I think reserve_additional_memory() should check the current resource
>> map and the e820 map to find a large enough unused region.  This can be
>> done as an additional patch at a later date.
>>
>>> It does not allow balloon driver to execute infinite
>>> loops when target exceeds limits in other cases too.
>>
>> This sentence confuses me.

I'm just confused by the English.  Perhaps it should say:

"The balloon driver will limit target to the maximum reservation as any
attempt to populate pages above the maximum reservation will always fail."

?

> That is why this patch is more generic.
> 
>>> Signed-off-by: Daniel Kiper <daniel.kiper@...cle.com>
>>> ---
>>>  drivers/xen/balloon.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index a56776d..07da753 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -65,6 +65,7 @@
>>>  #include <xen/balloon.h>
>>>  #include <xen/features.h>
>>>  #include <xen/page.h>
>>> +#include <xen/xenbus.h>
>>>
>>>  /*
>>>   * balloon_process() state:
>>> @@ -490,11 +491,55 @@ static void balloon_process(struct work_struct *work)
>>>  	mutex_unlock(&balloon_mutex);
>>>  }
>>>
>>> -/* Resets the Xen limit, sets new target, and kicks off processing. */
>>> +/* Enforce limits, set new target and kick off processing. */
>>>  void balloon_set_new_target(unsigned long target)
>>>  {
>>> +	domid_t domid = DOMID_SELF;
>>> +	int rc;
>>> +	unsigned long long host_limit;
>>> +
>>> +	/* Enforce statically defined limit. */
>>> +	target = min(target, MAX_DOMAIN_PAGES);
>>> +
>>> +	if (xen_initial_domain()) {
>>> +		rc = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
>>> +
>>> +		/* Limit is not enforced by hypervisor. */
>>> +		if (rc == -EPERM)
>>> +			goto no_host_limit;
>>> +
>>> +		if (rc <= 0) {
>>> +			pr_info("xen_balloon: %s: Initial domain target limit "
>>> +				"could not be established: %i\n", __func__, rc);
>>> +			goto no_host_limit;
>>> +		}
>>> +
>>> +		host_limit = rc;
>>
>> I think you should use this method for both dom0 and domUs.  No need to
>> check static-max from xenstore.
> 
> Sadly XENMEM_maximum_reservation for domU returns value which is set by xl mem-set
> not by xl mem-max :-(((... That is why I get this value from xenstore.

It gets d->max_pages which the limit for d->tot_pages. d->max_pages is
set by xl mem-max (and xl mem-set as it uses the enforce option to
libxl_set_memory_target()).

If you set the target above d->max_pages you won't be able to populate them.

So, using the maximum_reservation call seems like the right thing to me.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/