Re: [PATCH V6 2/2] xen/arm: Harden the P2M code in p2m_remove_mapping()

[Oleksandr <olekstysh@xxxxxxxxx>]

On 15.07.22 20:15, Julien Grall wrote:
> On 24/06/2022 16:31, Oleksandr wrote:
> > On 23.06.22 21:08, Julien Grall wrote:
> > > Hi Oleksandr,
> >
> > Hello Julien
> Hi Oleksandr,

Hello Julien



> > > On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > Borrow the x86's check from p2m_remove_page() which was added
> > > > by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e
> > > > "x86/p2m: don't assert that the passed in MFN matches for a remove"
> > > > and adjust it to the Arm code base.
> > > >
> > > > Basically, this check is strictly needed for the xenheap pages only
> > > > since there are several non-protected read accesses to our simplified
> > > > xenheap based M2P approach on Arm (most calls to 
> > > > page_get_xenheap_gfn()
> > > > are not protected by the P2M lock).
> > > To me, this read as you introduced a bug in patch #1 and now you are 
> > > fixing it. So this patch should have been first.
> >
> > Sounds like yes, I agree. But, in that case I propose to rewrite this 
> > text like the following:
> >
> > Basically, this check will be strictly needed for the xenheap pages 
> > only *and* only after applying subsequent
> NIT: s/only and only/, this is pretty clear that this patch is 
> necessary for a follow-up patch.
ok


> Also please add "a" in from of subsequent because the two patches may 
> not be committed together.
ok


> > commit which will introduce xenheap based M2P approach on Arm. But, 
> > it will be a good opportunity
> > to harden the P2M code for *every* RAM pages since it is possible to 
> > remove any GFN - MFN mapping
> > currently on Arm (even with the wrong helpers).
> [...]
>
> > > > Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > ---
> > > > You can find the corresponding discussion at:
> > > > https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xxxxxxx/ 
> > > >
> > > > Changes V5 -> V6:
> > > >   - new patch
> > > > ---
> > > >   xen/arch/arm/p2m.c | 21 +++++++++++++++++++++
> > > >   1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > index f87b48e..635e474 100644
> > > > --- a/xen/arch/arm/p2m.c
> > > > +++ b/xen/arch/arm/p2m.c
> > > > @@ -1311,11 +1311,32 @@ static inline int p2m_remove_mapping(struct 
> > > > domain *d,
> > > >                                        mfn_t mfn)
> > > >   {
> > > >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > > +    unsigned long i;
> > > >       int rc;
> > > >         p2m_write_lock(p2m);
> > > > +    for ( i = 0; i < nr; )
> > > One bit I really hate in the x86 code is the lack of in-code 
> > > documentation. It makes really difficult to understand the logic.
> > > I know this code was taken from x86, but I would like to avoid 
> > > making same mistake (this code is definitely not trivial). So can we 
> > > document the logic?
> >
> > ok, I propose the following text right after acquiring the p2m lock:
> >
> >
> >   /*
> >    * Before removing the GFN - MFN mapping for any RAM pages make sure
> >    * that there is no difference between what is already mapped and what
> >    * is requested to be unmapped. If passed mapping doesn't match
> >    * the existing one bail out early.
> NIT: I would simply write "If they don't match bail out early".
ok, I guess this is related to the last sentence only.


> Also, it would be good to explanation how this could happen. Something 
> like:
> "For instance, this could happen if two CPUs are requesting to unmap 
> the same P2M concurrently."
agree



> >    */
> >
> >
> > Could you please clarify, do you agree with both?
> I have proposed some changes in both cases. I originally thought I 
> would do the update in the commit. However, this is more than simple 
> tweak, so would you mind to send a new version?

yes, will do


> Cheers,
--
Regards,

Oleksandr Tyshchenko