Re: [Xen-devel] [PATCH v2] mm: Don't scrub pages while holding heap lock in alloc_heap_pages()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 09/05/2017 11:14 AM, Jan Beulich wrote:
>>>> On 05.09.17 at 16:54, <boris.ostrovsky@xxxxxxxxxx> wrote:
>> On 09/05/2017 10:42 AM, Boris Ostrovsky wrote:
>>>>> @@ -974,13 +972,39 @@ static struct page_info *alloc_heap_pages(
>>>>>           * guest can control its own visibility of/through the cache.
>>>>>           */
>>>>>          flush_page_to_ram(page_to_mfn(&pg[i]), !(memflags & 
>> MEMF_no_icache_flush));
>>>>> -
>>>>> -        if ( !(memflags & MEMF_no_scrub) )
>>>>> -            check_one_page(&pg[i]);
>>>>>      }
>>>>>  
>>>>>      spin_unlock(&heap_lock);
>>>>>  
>>>>> +    if ( first_dirty != INVALID_DIRTY_IDX ||
>>>>> +         (scrub_debug && !(memflags & MEMF_no_scrub)) )
>>>> Why does scrub_debug matter here?.
>>>>
>>>>> +    {
>>>>> +        for ( i = 0; i < (1U << order); i++ )
>>>>> +        {
>>>>> +            if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
>>>>> +            {
>>>>> +                if ( !(memflags & MEMF_no_scrub) )
>>>>> +                    scrub_one_page(&pg[i]);
>>>>> +
>>>>> +                dirty_cnt++;
>>>>> +
>>>>> +                spin_lock(&heap_lock);
>>>>> +                pg[i].count_info &= ~PGC_need_scrub;
>>>>> +                spin_unlock(&heap_lock);
>>>>> +            }
>>>>> +
>>>>> +            if ( !(memflags & MEMF_no_scrub) )
>>>>> +                check_one_page(&pg[i]);
>>>> Wouldn't this better be "else if", as checking a page just scrubbed
>>>> doesn't look very useful?
>>> For both of these questions --- we don't want to miss a poisoned page.
>>> For example, if a page was poisoned but for some reason is not marked
>>> PGC_need_scrub.
>>>
>>> Of course, we risk a false positive if a guest wrote the page with the
>>> same pattern.
>> Just in case I wasn't clear --- I will remove scrub_debug test and add
>> 'else' for this reason. Even though it's a debug-only feature I think we
>> shouldn't do this.
> Perhaps I'm dense, but it sounds to me like you agree to do a
> change you think you shouldn't do.

What I meant was that with current code we have a chance of a false
positive and that, even though it's a debug-only case, is probably not
the way to go.

If I make the change that you suggested this false positive will not
occur (although at the cost of reduced test coverage).

-boris


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