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

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

On 11/10/2017 5:49 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
> > @@ -4844,9 +4844,19 @@ int map_pages_to_xen(
> >               {
> >                   unsigned long base_mfn;
> >   
> > -                pl1e = l2e_to_l1e(*pl2e);
> >                   if ( locking )
> >                       spin_lock(&map_pgdir_lock);
> > +
> > +                /* Skip the re-consolidation if it's done on another CPU. */
> > +                if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
> > +                {
> > +                    if ( locking )
> > +                        spin_unlock(&map_pgdir_lock);
> > +                    goto check_l3;
> > +                }
> > +
> > +                ol2e = *pl2e;
> This wants moving up ahead of the if(), so you can use the local
> variable inside that if().
Got it. Thanks.

> > @@ -4880,6 +4889,15 @@ int map_pages_to_xen(
> >   
> >               if ( locking )
> >                   spin_lock(&map_pgdir_lock);
> > +
> > +            /* Skip the re-consolidation if it's done on another CPU. */
> > +            if ( l3e_get_flags(*pl3e) & _PAGE_PSE )
> > +            {
> > +                if ( locking )
> > +                    spin_unlock(&map_pgdir_lock);
> > +                continue;
> > +            }
> > +
> >               ol3e = *pl3e;
> Same here - move the if() below here and use ol3e in there.
>
> With that
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> I'm not certain this is important enough a fix to consider for 4.10,
> and you seem to think it's good enough if this gets applied only
> after the tree would be branched, as you didn't Cc Julien. Please
> indicate if you actually simply weren't aware, and you indeed
> there's an important aspect to this that I'm overlooking.
Well, at first I have not expected this to be accepted for 4.10. But 
since we have
met this issue in practice, when running a graphic application which 
consumes
memory intensively in dom0, I think it also makes sense if we can fix it 
in xen's
release as early as possible. Do you think this is a reasonable 
requirement? :-)
Also Cc to Julien. Julien, any comment? :-)

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

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