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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> 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@xxxxxxx>
> ---
>   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