Re: [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain

[Julien Grall <julien@xen.org>]

Hi,

On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> When destroyed domain is an owner domain of a static shared memory
> region, then we need to ensure that all according borrower domains
> shall not have the access to this static shared memory region too.

As Stefano wrote, I don't think this is necessary. The page reference 
accounting will keep the page alive until everyone have released the page.

So can you explain why you want to do that?

> 
> This commit covers above scenario through unmapping all borrowers'
> according foreign memory mapping when destroyed domain is a owner
> domain of a static shared memory region.
> 
> NOTE: It will best for users to destroy all borrowers before the owner
> domain in case encountering data abort when accidentally accessing
> the static shared memory region.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain.c | 88 ++++++++++++++++++++++++++++++++++---------
>   1 file changed, 71 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 73ffbfb918..8f4a8dcbfc 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -998,10 +998,39 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
>   }
>   
>   #ifdef CONFIG_STATIC_SHM
> +static int destroy_shm(struct domain *d, gfn_t gfn, unsigned long nr_gfns)
If you still plan to go ahead with this approach, then I would prefer if 
this function is created in patch #8. This will help to reduce the churn 
in this patch.

[...]

> -            for ( j = 0; j < nr_gfns; j++ )
> +            if ( test_bit(shm_id, shm_list_mask) )
>               {
> -                mfn_t mfn;
> -
> -                mfn = gfn_to_mfn(d, gfn_add(gfn, j));
> -                if ( !mfn_valid(mfn) )
> +                domid_t od = shm_list[shm_id].owner_dom;
> +                unsigned long j;
> +                /*
> +                 * If it is a owner domain, then after it gets destroyed,
> +                 * static shared memory region shall be unaccessible to all
> +                 * borrower domains too.
> +                 */
> +                if ( d->domain_id == od )
>                   {
> -                    dprintk(XENLOG_ERR,
> -                            "Domain %pd page number %lx invalid.\n",
> -                            d, gfn_x(gfn) + i);
> -                    return -EINVAL;
> +                    struct domain *bd;
> +
> +                    for ( j = 0; j < shm_list[shm_id].nr_borrower; j++ )
> +                    {
> +                        bd = get_domain_by_id(shm_list[shm_id].borrower_dom[j]);
> +                        /*
> +                         * borrower domain could be dead already, in such case
> +                         * no need to do the unmapping.

The domain ID could have been re-used. So it is not enough to lookup for 
the ID.

> +                         */
> +                        if ( bd != NULL )
> +                        {
> +                            gfn_t b_gfn = gaddr_to_gfn(
> +                                          shm_list[shm_id].borrower_gbase[j]);
> +                            ret = destroy_shm(bd, b_gfn, nr_gfns);
> +                            if ( ret )
> +                                dprintk(XENLOG_ERR,
> +                                        "Domain %pd: failed to destroy static shared memory.\n",
> +                                        bd);

In the commit message, you wrote you want to remove the pages from the 
borrower. But here, you will ignore a failure and continue to destroy 
the owner like nothing happened.

If you are concerned that the borrower can still use the pages. Then we 
should make sure that they are removed in every cases.

However, I think the code is going to quite complex. So I think we 
should consider to do nothing here and let the borrower use the pages 
until they die.

Potentially, we could notify the borrowers that the owner died so they 
can decide to remove/shutdown themself. Of course, it would mean we are 
relying on the borrowers to be nice. An alternative would be to destroy 
them.

Cheers,

-- 
Julien Grall