Re: [Xen-devel] [PATCH] x86/p2m: fixed p2m_change_type_range() start / end check

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.04.18 at 19:16, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -976,6 +976,13 @@ void p2m_change_type_range(struct domain *d,
>      ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
>  
>      p2m_lock(p2m);
> +
> +    if ( start > p2m->max_mapped_pfn )
> +    {
> +        p2m_unlock(p2m);
> +        return;
> +    }

I realize this is what George has suggested, but I still wonder if this is the
right thing to do here: Why is this any more important to check than the
more general start >= end (in which case the assertion in the rangeset
code would also trigger)? Till now the function assumes "sensible" things
to be passed in, but specifically also permitting the [start,~0UL] case
(just in a more generalizer fashion). The problem you're trying to fix here
is something passing in a nonsense range (starting above the valid range).
Yet if we want the function to be immune to nonsense being passed in, I
think start < end is what needs checking for, and that check would then
perhaps better go after end was already adjusted.

The obvious alternative is for callers to only pass in sane ranges (in which
case adding ASSERT() here may be considered, instead of triggering the
one in the rangeset code).

In any event you want to add unlikely(), just like the similar end check has.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel