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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.11.17 at 15:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> 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?

Oh, yes, definitely. I should have noticed this myself.

Jan


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