[ros-dev] [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...

[Timo Kreuzer]

I was thinking that a VAD with the Committed flag set would be entirely 
committed, so we could avoid all PTE checking. But I guess the flag 
stays set, even if we decommit ranges inside it. In that case it's clear 
that we need to check for decommitted pages.

Regarding your change, I see the issue now (I was misinterpreting it before)
Your fix looks correct to me.

Timo

Am 21.02.2015 um 16:41 schrieb Jerome Gardou:
> It is possible that the entire VAD range is not committed, so we have 
> to sweep over all the PTEs in the sub-range to check. If one of the 
> PTEs is zero AND the entire VAD is not marked as committed, then we're 
> sure that the range is not committed. Also, if the PTE is marked as 
> decommitted, same thing happen.
>
> So, to check the PTEs, we have to make sure the underlying PDE is 
> there (== not paged out). Otherwise, by accessing a PTE, the CPU might 
> raise a page fault while we're holding the process working set lock 
> --> failed ASSERT.
> Notice that if the PDE is zero, it's not faulted in and we check for 
> the VAD commit flag. (like we do for the PTEs later in the loop iteration)
>
> If I misunderstood something, I'd be happy to be given some hint as 
> why this patch is incorrect.
>
> Regards
> Jérôme
>
> Le 20/02/2015 22:00, Timo Kreuzer a écrit :
>>
>> Why does the PDE need to be faulted in? The function is called 
>> MiIsEntireRangeCommitted, not 
>> MiCheckIfEntireRangeIsCommittedAndMakePdeValid.
>> I also wonder why one would even walk the PTEs if the VAD is MEM_COMMIT.
>>
>>
>>
>> Am 19.02.2015 um 11:19 schrieb Jérôme Gardou:
>>> If it takes me 6 months to understand why, then yes.
>>>
>>> Hint: you can make this delay shorter.
>>>
>>> Also the title is a bit misleading, it's the PDE that is not faulted in
>>> in case we don't terminate the loop iteration early.
>>>
>>> Le 19/02/2015 05:46, Alex Ionescu a écrit :
>>>> it might fix an assert but the patch is incorrect. will this also 
>>>> take 6
>>>> months to revert?
>>>>
>>>> Best regards,
>>>> Alex Ionescu
>>>>
>>>> On Tue, Feb 17, 2015 at 6:19 AM, <jgardou at svn.reactos.org> wrote:
>>>>
>>>>> Author: jgardou
>>>>> Date: Tue Feb 17 14:19:05 2015
>>>>> New Revision: 66334
>>>>>
>>>>> URL: http://svn.reactos.org/svn/reactos?rev=66334&view=rev
>>>>> Log:
>>>>> [NTOSKRNL/MM]
>>>>>   - MiIsEntireRangeCommitted: Ensure the PTE we are checking is 
>>>>> really
>>>>> faulted in.
>>>>>   - Prefer MiPteToPde and MiPdeToPte (which should really be called
>>>>> MiFirstPteInPde) instead of MiAddressToPte and MiPteToAddress
>>>>> Fixes weird failed ASSERT in page fault handler when using DPH.
>>>>>
>>>>> Modified:
>>>>>      trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
>>>>>
>>>>> Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
>>>>> URL:
>>>>> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=66334&r1=66333&r2=66334&view=diff 
>>>>>
>>>>>
>>>>> ============================================================================== 
>>>>>
>>>>> --- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original)
>>>>> +++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Tue Feb 17
>>>>> 14:19:05 2015
>>>>> @@ -1994,14 +1994,13 @@
>>>>>           if (OnBoundary)
>>>>>           {
>>>>>               /* Is this PDE demand zero? */
>>>>> -            PointerPde = MiAddressToPte(PointerPte);
>>>>> +            PointerPde = MiPteToPde(PointerPte);
>>>>>               if (PointerPde->u.Long != 0)
>>>>>               {
>>>>>                   /* It isn't -- is it valid? */
>>>>>                   if (PointerPde->u.Hard.Valid == 0)
>>>>>                   {
>>>>>                       /* Nope, fault it in */
>>>>> -                    PointerPte = MiPteToAddress(PointerPde);
>>>>>                       MiMakeSystemAddressValid(PointerPte, Process);
>>>>>                   }
>>>>>               }
>>>>> @@ -2009,13 +2008,13 @@
>>>>>               {
>>>>>                   /* The PTE was already valid, so move to the 
>>>>> next one */
>>>>>                   PointerPde++;
>>>>> -                PointerPte = MiPteToAddress(PointerPde);
>>>>> +                PointerPte = MiPdeToPte(PointerPde);
>>>>>
>>>>>                   /* Is the entire VAD committed? If not, fail */
>>>>>                   if (!Vad->u.VadFlags.MemCommit) return FALSE;
>>>>>
>>>>> -                /* Everything is committed so far past the range, 
>>>>> return
>>>>> true */
>>>>> -                if (PointerPte > LastPte) return TRUE;
>>>>> +                /* New loop iteration with our new, on-boundary 
>>>>> PTE. */
>>>>> +                continue;
>>>>>               }
>>>>>           }
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Ros-dev mailing list
>>>> Ros-dev at reactos.org
>>>> http://www.reactos.org/mailman/listinfo/ros-dev
>>>>
>>> _______________________________________________
>>> Ros-dev mailing list
>>> Ros-dev at reactos.org
>>> http://www.reactos.org/mailman/listinfo/ros-dev
>>>
>>
>>
>>
>>
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20150221/75431dc6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3683 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20150221/75431dc6/attachment.bin>