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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-11-13 at 14:12 +0000, Jan Beulich wrote:
> >>> On 13.11.13 at 14:34, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 
> > 
> > On 11/13/2013 01:26 PM, Jan Beulich wrote:
> >>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> CC: Keir Fraser <keir@xxxxxxx>
> >>> CC: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>> ---
> >>>      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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel