[PATCH V2] xen: Check if the range is valid in init_domheap_pages

[PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Julien Grall]

On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in
the device tree.
In this case, when xen decides to free unused memory, dt_unreserved_regions
will call init_domheap_pages with the start and the end of range equals. But
the latter assumes that (start > end), if not Xen will hang because the
number of pages is equals to (unsigned)-1.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>

---
    Changes in v2:
        - Change commit title
        - Move the check in init_domheap_pages
---
 xen/common/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 4c17fbd..10f67a1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
     smfn = round_pgup(ps) >> PAGE_SHIFT;
     emfn = round_pgdown(pe) >> PAGE_SHIFT;
 
+    if ( smfn <= emfn )
+        return;
+
     init_heap_pages(mfn_to_page(smfn), emfn - smfn);
 }
 
-- 
1.8.3.1

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Ian Campbell]

On Wed, 2013-11-13 at 13:15 +0000, Julien Grall wrote:
> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in
> the device tree.
> In this case, when xen decides to free unused memory, dt_unreserved_regions
> will call init_domheap_pages with the start and the end of range equals. But
> the latter assumes that (start > end), if not Xen will hang because the
> number of pages is equals to (unsigned)-1.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> 
> ---
>     Changes in v2:
>         - Change commit title
>         - Move the check in init_domheap_pages
> ---
>  xen/common/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 4c17fbd..10f67a1 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>      smfn = round_pgup(ps) >> PAGE_SHIFT;
>      emfn = round_pgdown(pe) >> PAGE_SHIFT;
>  
> +    if ( smfn <= emfn )

You've got this backwards I think.

> +        return;
> +
>      init_heap_pages(mfn_to_page(smfn), emfn - smfn);
>  }
>  

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Jan Beulich]

>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote:
> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory 
> in the device tree.
> In this case, when xen decides to free unused memory, dt_unreserved_regions
> will call init_domheap_pages with the start and the end of range equals. But
> the latter assumes that (start > end), if not Xen will hang because the
> number of pages is equals to (unsigned)-1.

The change is simple enough, so I don't really mind it going in, but
I wonder ...

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> 
> ---
>     Changes in v2:
>         - Change commit title
>         - Move the check in init_domheap_pages

... who and why suggested to move it here. After all, I'm considering
it an error to call the function with non-page-aligned addresses and/
or end < start (I take it that page-aligned, but start == end is not a
problem without your change).

Jan

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>      smfn = round_pgup(ps) >> PAGE_SHIFT;
>      emfn = round_pgdown(pe) >> PAGE_SHIFT;
>  
> +    if ( smfn <= emfn )
> +        return;
> +
>      init_heap_pages(mfn_to_page(smfn), emfn - smfn);
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Julien Grall]

On 11/13/2013 01:26 PM, Jan Beulich wrote:
>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote:
>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory
>> in the device tree.
>> In this case, when xen decides to free unused memory, dt_unreserved_regions
>> will call init_domheap_pages with the start and the end of range equals. But
>> the latter assumes that (start > end), if not Xen will hang because the
>> number of pages is equals to (unsigned)-1.
>
> The change is simple enough, so I don't really mind it going in, but
> I wonder ...
>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>>      Changes in v2:
>>          - Change commit title
>>          - Move the check in init_domheap_pages
>
> ... who and why suggested to move it here. After all, I'm considering
> it an error to call the function with non-page-aligned addresses and/
> or end < start (I take it that page-aligned, but start == end is not a
> problem without your change).

if ps == pe, then emfn == (smfn - 1). This will result to the number of 
pages of -1.

There is a similar check in init_xenheap_pages, it doesn't seem harmfull 
to let it here.

-- 
Julien Grall

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Julien Grall]

On 11/13/2013 01:23 PM, Ian Campbell wrote:
> On Wed, 2013-11-13 at 13:15 +0000, Julien Grall wrote:
>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in
>> the device tree.
>> In this case, when xen decides to free unused memory, dt_unreserved_regions
>> will call init_domheap_pages with the start and the end of range equals. But
>> the latter assumes that (start > end), if not Xen will hang because the
>> number of pages is equals to (unsigned)-1.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>>      Changes in v2:
>>          - Change commit title
>>          - Move the check in init_domheap_pages
>> ---
>>   xen/common/page_alloc.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 4c17fbd..10f67a1 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
>>       smfn = round_pgup(ps) >> PAGE_SHIFT;
>>       emfn = round_pgdown(pe) >> PAGE_SHIFT;
>>
>> +    if ( smfn <= emfn )
>
> You've got this backwards I think.

Oh right, I will fix it in the next version.

>
>> +        return;
>> +
>>       init_heap_pages(mfn_to_page(smfn), emfn - smfn);
>>   }
>>
>
>

-- 
Julien Grall

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Ian Campbell]

On Wed, 2013-11-13 at 13:26 +0000, Jan Beulich wrote:
> >>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote:
> > On ARM, when an initrd is given to xen by U-boot, it will reserve the memory 
> > in the device tree.
> > In this case, when xen decides to free unused memory, dt_unreserved_regions
> > will call init_domheap_pages with the start and the end of range equals. But
> > the latter assumes that (start > end), if not Xen will hang because the
> > number of pages is equals to (unsigned)-1.
> 
> The change is simple enough, so I don't really mind it going in, but
> I wonder ...
> 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > 
> > ---
> >     Changes in v2:
> >         - Change commit title
> >         - Move the check in init_domheap_pages
> 
> ... who and why suggested to move it here.

Me in response to "xen/arm: Don't call init_domheap_page with an empty
range".

>  After all, I'm considering
> it an error to call the function with non-page-aligned addresses and/
> or end < start (I take it that page-aligned, but start == end is not a
> problem without your change).

Since init_xenheap_page does the right thing it seemed reasonable to me
to make domheap do the same.

Ian,

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Jan Beulich]

>>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote:

> 
> On 11/13/2013 01:26 PM, Jan Beulich wrote:
>>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote:
>>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory
>>> in the device tree.
>>> In this case, when xen decides to free unused memory, dt_unreserved_regions
>>> will call init_domheap_pages with the start and the end of range equals. But
>>> the latter assumes that (start > end), if not Xen will hang because the
>>> number of pages is equals to (unsigned)-1.
>>
>> The change is simple enough, so I don't really mind it going in, but
>> I wonder ...
>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Change commit title
>>>          - Move the check in init_domheap_pages
>>
>> ... who and why suggested to move it here. After all, I'm considering
>> it an error to call the function with non-page-aligned addresses and/
>> or end < start (I take it that page-aligned, but start == end is not a
>> problem without your change).
> 
> if ps == pe, then emfn == (smfn - 1). This will result to the number of 
> pages of -1.

I'm not following: If ps == pe and they're page aligned, then

    smfn = round_pgup(ps) >> PAGE_SHIFT;
    emfn = round_pgdown(pe) >> PAGE_SHIFT;

produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires
the input to either be not page aligned, or pe < ps, both of which look
invalid to me.

> There is a similar check in init_xenheap_pages, it doesn't seem harmfull 
> to let it here.

In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for
a good reason - that code doesn't tolerate ps == pe (and if I'm not
mistaken would break even on specific ps < pe cases).

Jan

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Ian Campbell]

On Wed, 2013-11-13 at 14:12 +0000, Jan Beulich wrote:
> >>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote:
> 
> > 
> > On 11/13/2013 01:26 PM, Jan Beulich wrote:
> >>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote:
> >>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory
> >>> in the device tree.
> >>> In this case, when xen decides to free unused memory, dt_unreserved_regions
> >>> will call init_domheap_pages with the start and the end of range equals. But
> >>> the latter assumes that (start > end), if not Xen will hang because the
> >>> number of pages is equals to (unsigned)-1.
> >>
> >> The change is simple enough, so I don't really mind it going in, but
> >> I wonder ...
> >>
> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>> CC: Keir Fraser <keir@xen.org>
> >>> CC: Jan Beulich <jbeulich@suse.com>
> >>>
> >>> ---
> >>>      Changes in v2:
> >>>          - Change commit title
> >>>          - Move the check in init_domheap_pages
> >>
> >> ... who and why suggested to move it here. After all, I'm considering
> >> it an error to call the function with non-page-aligned addresses and/
> >> or end < start (I take it that page-aligned, but start == end is not a
> >> problem without your change).
> > 
> > if ps == pe, then emfn == (smfn - 1). This will result to the number of 
> > pages of -1.
> 
> I'm not following: If ps == pe and they're page aligned, then
> 
>     smfn = round_pgup(ps) >> PAGE_SHIFT;
>     emfn = round_pgdown(pe) >> PAGE_SHIFT;
> 
> produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires
> the input to either be not page aligned, or pe < ps, both of which look
> invalid to me.
> 
> > There is a similar check in init_xenheap_pages, it doesn't seem harmfull 
> > to let it here.
> 
> In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for
> a good reason - that code doesn't tolerate ps == pe (and if I'm not
> mistaken would break even on specific ps < pe cases).

init_xenheap_pages doesn't tolerate it, so it checks for it instead of
pushing the burden onto the caller.

Julien is adding the same check to init_domheap_pages which also doesn't
tolerate it, but here you are arguing that it should be up to the caller
to not pass invalid parameters.

This could be fixed in the caller, but it seems cleaner to do it in the
core to me, since there are plenty of case where you are munging around
with addresses and catching all the corners where that munging ends up
makes end<=start makes that code uglier.

Ian.

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Jan Beulich]

>>> On 13.11.13 at 15:18, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2013-11-13 at 14:12 +0000, Jan Beulich wrote:
>> >>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote:
>> > There is a similar check in init_xenheap_pages, it doesn't seem harmfull 
>> > to let it here.
>> 
>> In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for
>> a good reason - that code doesn't tolerate ps == pe (and if I'm not
>> mistaken would break even on specific ps < pe cases).
> 
> init_xenheap_pages doesn't tolerate it, so it checks for it instead of
> pushing the burden onto the caller.
> 
> Julien is adding the same check to init_domheap_pages which also doesn't
> tolerate it, but here you are arguing that it should be up to the caller
> to not pass invalid parameters.

Not exactly: init_xenheap_pages() wants to _shrink_ the area
(under certain conditions), and hence needs to check even for
the valid (from a calling perspective) case of ps == pe.

> This could be fixed in the caller, but it seems cleaner to do it in the
> core to me, since there are plenty of case where you are munging around
> with addresses and catching all the corners where that munging ends up
> makes end<=start makes that code uglier.

And again, I'm not fundamentally opposed to the patch (and it's
anyway Keir who'll need to judge on whether to take it), I'm just
trying to point out that there must be something wrong with
how the function is being called (and that if it was called with
proper arguments, the issue would have shown up in the first
place).

Jan

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Julien Grall]

On 11/13/2013 02:12 PM, Jan Beulich wrote:
>>>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote:
>
> I'm not following: If ps == pe and they're page aligned, then
>
>      smfn = round_pgup(ps) >> PAGE_SHIFT;
>      emfn = round_pgdown(pe) >> PAGE_SHIFT;
>
> produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires
> the input to either be not page aligned, or pe < ps, both of which look
> invalid to me.

Right, my fault I didn't pay attention that ps and pe is non-aligned, 
with the same value. That will result to my previous assertion.

In any case, init_domheap_pages seems to be able to handle non-aligned 
address. Otherwise round_pg{up,down} is not necessary.

-- 
Julien Grall

Re: [PATCH V2] xen: Check if the range is valid in init_domheap_pages

[Ian Campbell]

On Wed, 2013-11-13 at 14:32 +0000, Jan Beulich wrote:
> I'm just
> trying to point out that there must be something wrong with
> how the function is being called (and that if it was called with
> proper arguments, the issue would have shown up in the first
> place).

I don't think any one has disputed that the inputs are invalid or that
the caller could be changed instead.

I think it is better to make this function more liberal in what it
accepts (by rejecting certain invalid constructs) than to add complexity
to all the callers.

Ian.