Re: [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain

[Julien Grall <julien@xen.org>]

Hi Penny,

On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> This commit introduces a new helper destroy_domain_shm to destroy static
> shared memory at domain de-construction.
> 
> This patch only considers the scenario where the owner domain is the
> default dom_shared, for user-defined owner domain, it will be covered in
> the following patches.
> 
> Since all domains are borrower domains, we could simply remove guest P2M
> foreign mapping of statically shared memory region and drop the reference
> added at guest_physmap_add_shm.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1ff1df5d3f..f0bfd67fe5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -34,6 +34,7 @@
>   #include <asm/platform.h>
>   #include <asm/procinfo.h>
>   #include <asm/regs.h>
> +#include <asm/setup.h>
>   #include <asm/tee/tee.h>
>   #include <asm/vfp.h>
>   #include <asm/vgic.h>
> @@ -993,6 +994,48 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
>       return ret;
>   }
>   
> +#ifdef CONFIG_STATIC_SHM
> +static int domain_destroy_shm(struct domain *d)
> +{
> +    int ret = 0;
> +    unsigned long i = 0UL, j;
> +
> +    if ( d->arch.shm_mem == NULL )
> +        return ret;

You already return the value here. So...

> +    else

... the else is pointless.

> +    {
> +        for ( ; i < d->arch.shm_mem->nr_banks; i++ )
> +        {
> +            unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem->bank[i].size);
> +            gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start);
> +
> +            for ( j = 0; j < nr_gfns; j++ )
> +            {
> +                mfn_t mfn;
> +
> +                mfn = gfn_to_mfn(d, gfn_add(gfn, j));

A domain is allowed to modify its P2M. So there are no guarantee that 
the GFN will still point to the shared memory. This will allow the guest...

> +                if ( !mfn_valid(mfn) )
> +                {
> +                    dprintk(XENLOG_ERR,
> +                            "Domain %pd page number %lx invalid.\n",
> +                            d, gfn_x(gfn) + i);
> +                    return -EINVAL;

... to actively prevent destruction.

> +                }


> +
> +                ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0);
> +                if ( ret )
> +                    return ret;
> +
> +                /* Drop the reference. */
> +                put_page(mfn_to_page(mfn));

guest_physmap_remove_page() will already drop the reference taken for 
the foreign mapping. I couldn't find any other reference taken, what is 
the put_page() for?

Also, as per above we don't know whether this is a page from the shared 
page. So we can't blindly call put_page().

However, I don't think we need any specific code here. We can rely on 
relinquish_p2m_mappings() to drop any reference. If there is an extra 
one for shared mappings, then we should update p2m_put_l3_page().

Cheers,

-- 
Julien Grall