Re: [PATCH v9 01/14] arch/arm: add consistency check to REMOVE p2m changes

[Julien Grall <julien.grall@linaro.org>]

Hi Arianna,

On 07/02/2014 07:42 PM, Arianna Avanzini wrote:
>  xen/arch/arm/p2m.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 9960e17..7cb4a27 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c

[..]

> @@ -439,12 +441,37 @@ static int apply_p2m_changes(struct domain *d,
>                      pte = mfn_to_p2m_entry(maddr >> PAGE_SHIFT, mattr, t);
>                      p2m_write_pte(&third[third_table_offset(addr)],
>                                    pte, flush_pt);
> -                    maddr += PAGE_SIZE;
>                  }
>                  break;
> -            case RELINQUISH:
>              case REMOVE:
>                  {
> +                    unsigned long mfn = pte.p2m.base;
> +
> +                    /*
> +                     * Ensure that the guest address addr currently being
> +                     * handled (that is in the range given as argument to
> +                     * this function) is actually mapped to the corresponding
> +                     * machine address in the specified range. maddr here is
> +                     * the machine address given to the function, while mfn
> +                     * is the machine frame number actually mapped to the
> +                     * guest address: check if the two correspond.
> +                     */
> +                    if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> +                    {
> +                        gdprintk(XENLOG_WARNING,
> +                                 "p2m_remove: mapping at %"PRIpaddr" is of maddr %"PRIpaddr" not %"PRIpaddr" as expected",
> +                                 addr, pfn_to_paddr(mfn), maddr);
> +                        /*
> +                         * Continue to process the range even if an error is
> +                         * encountered, to prevent I/O-memory regions from
> +                         * being partially accessible to a domain.
> +                         */
> +                       continue;

This is buggy, you never update addr and maddr. So if a mapping is not
there, the code will end up in infinite loop.

Regards,

-- 
Julien Grall