Re: [PATCH v2] mm/hugetlb: fix a addressing exception caused by huge_pte_offset()

["Longpeng (Mike, Cloud Infrastructure Service Product Dept.)" <longpeng2@huawei.com>]

On 2020/3/24 6:52, Jason Gunthorpe wrote:
> On Mon, Mar 23, 2020 at 01:35:07PM -0700, Mike Kravetz wrote:
>> On 3/23/20 11:07 AM, Jason Gunthorpe wrote:
>>> On Mon, Mar 23, 2020 at 10:27:48AM -0700, Mike Kravetz wrote:
>>>
>>>>>  	pgd = pgd_offset(mm, addr);
>>>>> -	if (!pgd_present(*pgd))
>>>>> +	if (!pgd_present(READ_ONCE(*pgd)))
>>>>>  		return NULL;
>>>>>  	p4d = p4d_offset(pgd, addr);
>>>>> -	if (!p4d_present(*p4d))
>>>>> +	if (!p4d_present(READ_ONCE(*p4d)))
>>>>>  		return NULL;
>>>>>  
>>>>>       pud = pud_offset(p4d, addr);
>>>>
>>>> One would argue that pgd and p4d can not change from present to !present
>>>> during the execution of this code.  To me, that seems like the issue which
>>>> would cause an issue.  Of course, I could be missing something.
>>>
>>> This I am not sure of, I think it must be true under the read side of
>>> the mmap_sem, but probably not guarenteed under RCU..
>>>
>>> In any case, it doesn't matter, the fact that *p4d can change at all
>>> is problematic. Unwinding the above inlines we get:
>>>
>>>   p4d = p4d_offset(pgd, addr)
>>>   if (!p4d_present(*p4d))
>>>       return NULL;
>>>   pud = (pud_t *)p4d_page_vaddr(*p4d) + pud_index(address);
>>>
>>> According to our memory model the compiler/CPU is free to execute this
>>> as:
>>>
>>>   p4d = p4d_offset(pgd, addr)
>>>   p4d_for_vaddr = *p4d;
>>>   if (!p4d_present(*p4d))
>>>       return NULL;
>>>   pud = (pud_t *)p4d_page_vaddr(p4d_for_vaddr) + pud_index(address);
>>>
>>
>> Wow!  How do you know this?  You don't need to answer :)
> 
> It says explicitly in Documentation/memory-barriers.txt - see
> section COMPILER BARRIER:
> 
>  (*) The compiler is within its rights to reorder loads and stores
>      to the same variable, and in some cases, the CPU is within its
>      rights to reorder loads to the same variable.  This means that
>      the following code:
> 
>         a[0] = x;
>         a[1] = x;
> 
>      Might result in an older value of x stored in a[1] than in a[0].
> 
> It also says READ_ONCE puts things in program order, but we don't use
> READ_ONCE inside pud_offset(), so it doesn't help us.
> 
> Best answer is to code things so there is exactly one dereference of
> the pointer protected by READ_ONCE. Very clear to read, very safe.
> 
> Maybe Longpeng can rework the patch around these principles?
> 
Thanks Jason and Mike, I learn a lot from your analysis.

So... the patch should like this ?

@@ -4909,29 +4909,33 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
 		       unsigned long addr, unsigned long sz)
 {
 	pgd_t *pgd;
-	p4d_t *p4d;
-	pud_t *pud;
-	pmd_t *pmd;
+	p4d_t *p4g, p4d_entry;
+	pud_t *pud, pud_entry;
+	pmd_t *pmd, pmd_entry;

 	pgd = pgd_offset(mm, addr);
 	if (!pgd_present(*pgd))
 		return NULL;
-	p4d = p4d_offset(pgd, addr);
-	if (!p4d_present(*p4d))
+
+	p4g = p4d_offset(pgd, addr);
+	p4d_entry = READ_ONCE(*p4g);
+	if (!p4d_present(p4d_entry))
 		return NULL;

-	pud = pud_offset(p4d, addr);
-	if (sz != PUD_SIZE && pud_none(*pud))
+	pud = pud_offset(&p4d_entry, addr);
+	pud_entry = READ_ONCE(*pud);
+	if (sz != PUD_SIZE && pud_none(pud_entry))
 		return NULL;
 	/* hugepage or swap? */
-	if (pud_huge(*pud) || !pud_present(*pud))
+	if (pud_huge(pud_entry) || !pud_present(pud_entry))
 		return (pte_t *)pud;

-	pmd = pmd_offset(pud, addr);
-	if (sz != PMD_SIZE && pmd_none(*pmd))
+	pmd = pmd_offset(&pud_entry, addr);
+	pmd_entry = READ_ONCE(*pmd);
+	if (sz != PMD_SIZE && pmd_none(pmd_entry))
 		return NULL;
 	/* hugepage or swap? */
-	if (pmd_huge(*pmd) || !pmd_present(*pmd))
+	if (pmd_huge(pmd_entry) || !pmd_present(pmd_entry))
 		return (pte_t *)pmd;

> Also I wonder if the READ_ONCE(*pmdp) is OK. gup_pmd_range() uses it,
> but I can't explain why it shouldn't be pmd_read_atomic().
> 
>>> In the case where p4 goes from !present -> present (ie
>>> handle_mm_fault()):
>>>
>>> p4d_for_vaddr == p4d_none, and p4d_present(*p4d) == true, meaning the
>>> p4d_page_vaddr() will crash.
>>>
>>> Basically the problem here is not just missing READ_ONCE, but that the
>>> p4d is read multiple times at all. It should be written like gup_fast
>>> does, to guarantee a single CPU read of the unstable data:
>>>
>>>   p4d = READ_ONCE(*p4d_offset(pgdp, addr));
>>>   if (!p4d_present(p4))
>>>       return NULL;
>>>   pud = pud_offset(&p4d, addr);
>>>
>>> At least this is what I've been able to figure out :\
>>
>> In that case, I believe there are a bunch of similar routines with this issue.
> 
> Yes, my look around page walk related users makes me come to a similar
> worry.
> 
> Fortunately, I think this is largely theoretical as most likely the
> compiler will generate a single store for these coding patterns. 
> 
> That said, there have been bugs in the past, see commit 26c191788f18
> ("mm: pmd_read_atomic: fix 32bit PAE pmd walk vs pmd_populate SMP race
> condition") which is significantly related to the compiler lifting a
> load inside pte_offset to before the required 'if (pmd_*)' checks.
> 
>> For this patch, I was primarily interested in seeing the obvious
>> multiple dereferences in C fixed up.  This is above and beyond that!
>> :)
> 
> Well, I think it is worth solving the underlying problem
> properly. Otherwise we get weird solutions to data races like
> pmd_trans_unstable()...
> 
> Jason
> .
> 

---
Regards,
Longpeng(Mike)