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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.11.13 at 15:18, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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:
>> > 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


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