Re: [Xen-devel] [PATCH] x86/mm: fix a potential race condition in map_pages_to_xen().

[Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 11/9/2017 5:19 PM, Jan Beulich wrote:
> > > > On 09.11.17 at 16:29, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4844,9 +4844,10 @@ int map_pages_to_xen(
> >               {
> >                   unsigned long base_mfn;
> >   
> > -                pl1e = l2e_to_l1e(*pl2e);
> >                   if ( locking )
> >                       spin_lock(&map_pgdir_lock);
> > +
> > +                pl1e = l2e_to_l1e(*pl2e);
> >                   base_mfn = l1e_get_pfn(*pl1e) & ~(L1_PAGETABLE_ENTRIES - 1);
> >                   for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++, pl1e++ )
> >                       if ( (l1e_get_pfn(*pl1e) != (base_mfn + i)) ||
> I agree with the general observation, but there are three things I'd
> like to see considered:
>
> 1) Please extend the change slightly such that the L2E
> re-consolidation code matches the L3E one (i.e. latch into ol2e
> earlier and pass that one to l2e_to_l1e(). Personally I would even
> prefer if the presence/absence of blank lines matched between
> the two pieces of code.
Got it. Thanks.

> 2) Is your change actually enough to take care of all forms of the
> race you describe? In particular, isn't it necessary to re-check PSE
> after having taken the lock, in case another CPU has just finished
> doing the re-consolidation?
Good question. :-)

I'd thought of checking the PSE for pl2e, and dropped that. My understanding
was below:
After the lock is taken, pl2e will be pointing to either a L1 page table 
in normal
cases; or to a superpage if another CPU has just finished the 
re-consolidation
and released the lock. And for the latter scenario, l1e_get_pfn(*pl1e) 
shall not
be equal to (base_mfn + i), and will not jump out the the loop.

But after second thought, above understanding is based on assumption of the
contents of the target superpage. No matter how small the chance is, we can
not make such assumption.

So my suggestion is we add the check the PSE and if it is set, "goto 
check_l3".
Is this reasonable to you?

> 3) What about the empty&free checks in modify_xen_mappings()?
Oh. Thanks for the remind.
Just had a look. It seems pl1e or pl2e may be freed more than once for the
empty & free checks, due to lack of protection.
So we'd better add a lock too, right?

Yu


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel