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

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

On 11/10/2017 5:57 PM, Jan Beulich wrote:
> > > > On 10.11.17 at 08:18, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5097,6 +5097,17 @@ int modify_xen_mappings(unsigned long s, unsigned long 
> > e, unsigned int nf)
> >                */
> >               if ( (nf & _PAGE_PRESENT) || ((v != e) && (l1_table_offset(v) != 
> > 0)) )
> >                   continue;
> > +            if ( locking )
> > +                spin_lock(&map_pgdir_lock);
> > +
> > +            /* L2E may be cleared on another CPU. */
> > +            if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
> I think you also need a PSE check here, or else the l2e_to_l1e() below
> may be illegal.
Hmm, interesting point, and thanks! :-)
I did not check the PSE, because modify_xen_mappings() will not do the 
re-consolidation, and
concurrent invokes of this routine will not change this flag. But now I 
believe this presumption
shall not be made, because the paging structures may be modified by 
other routines, like
map_pages_to_xen() on other CPUs.

So yes, I think a _PAGE_PSE check is necessary here. And I suggest we 
also check the _PAGE_PRESENT
flag as well, for the re-consolidation part in my first patch for 
map_pages_to_xen(). Do you agree?
> > @@ -5105,11 +5116,16 @@ int modify_xen_mappings(unsigned long s, unsigned long 
> > e, unsigned int nf)
> >               {
> >                   /* Empty: zap the L2E and free the L1 page. */
> >                   l2e_write_atomic(pl2e, l2e_empty());
> > +                if ( locking )
> > +                    spin_unlock(&map_pgdir_lock);
> >                   flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
> >                   free_xen_pagetable(pl1e);
> >               }
> > +            else if ( locking )
> > +                spin_unlock(&map_pgdir_lock);
> >           }
> >   
> > +check_l3:
> Labels indented by at least one space please.
Got it . Thanks.

Yu
> Jan

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