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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi, julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, April 9, 2022 5:26 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v1 08/13] xen/arm: destroy static shared memory when
> de-construct domain
> 
> 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().
> 

True, true. Thanks for pointing this out!
In p2m_put_l3_page, it has already called put_page() for foreign mapping,
definitely no extra one here!

> Cheers,
> 
> --
> Julien Grall