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

[Mike Kravetz]

On 2/22/20 5:24 PM, Longpeng (Mike) wrote:
> å 2020/2/23 1:02, Matthew Wilcox åé:
>> On Sat, Feb 22, 2020 at 02:33:10PM +0800, Longpeng (Mike) wrote:
>>> å 2020/2/22 13:23, Qian Cai åé:
>>>>> On Feb 21, 2020, at 10:34 PM, Longpeng(Mike) <longpeng2@xxxxxxxxxx> wrote:
>>>>>
>>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>>> index dd8737a..90daf37 100644
>>>>> --- a/mm/hugetlb.c
>>>>> +++ b/mm/hugetlb.c
>>>>> @@ -4910,28 +4910,30 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>> {
>>>>>    pgd_t *pgd;
>>>>>    p4d_t *p4d;
>>>>> -    pud_t *pud;
>>>>> -    pmd_t *pmd;
>>>>> +    pud_t *pud, pud_entry;
>>>>> +    pmd_t *pmd, pmd_entry;
>>>>>
>>>>>    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;
>>>>
>>>> Whatâs the point of READ_ONCE() on those two places?
>>>>
>>> As explained in the commit messages, it's for safe(e.g. avoid the compilier
>>> mischief). You can also find the same usage in the ARM64's huge_pte_offset() in
>>> arch/arm64/mm/hugetlbpage.c
>>
>> I rather agree with Qian; if we need something like READ_ONCE() here,
>> why don't we always need it as part of pgd_present()?  It seems like an
>> unnecessary burden for every user.
>>
> Hi Matthew & Qian,
> 
> Firstly, this is NOT a 'blindly copy', it's an unwise words. I don't know
> whether you read the commit message (commit 20a004e7) of ARM64's huge_pte_offset
> ? If you read, I think worry about the safe is necessary.
> 
> Secondly, huge_pte_offset in mm/hugetlb.c is for ARCH_WANT_GENERAL_HUGETLB, many
> architectures use it, can you make sure there is no issue on all the
> architectures using it with all the version of gcc ?
> 
> Thirdly, there are several places use READ_ONCE to access the page table in mm/*
> (e.g. gup_pmd_range), they're also generical for all architectures, and they're
> much more like unnecessary than here, so why there can use but not here? What's
> more, you can read this commit 688272809.

Apologies for the late reply.

In commit 20a004e7 the message says that "Whilst there are some scenarios
where this cannot happen ... the overhead of using READ_ONCE/WRITE_ONCE
everywhere is minimal and makes the code an awful lot easier to reason about."
Therefore, a decision was made to ALWAYS use READ_ONCE in the arm64 code
whether or not it was absolutely necessary.  Therefore, I do not think
we can assume all the READ_ONCE additions made in 20a004e7 are necessary.
Then the question remains, it it necessary in two statements above?
I do not believe it is necessary.  Why?  In the statements,
	if (!pgd_present(*pgd))
and
	if (!p4d_present(*p4d))
the variables are only accessed and dereferenced once.  I can not imagine
any way in which the compiler could perform multiple accesses of the variable.

I do believe the READ_ONCE in code accessing the pud and pmd is necessary.
This is because the variables (pud_entry or pmd_entry) are accessed more than
once.  And, I could imagine some strange compiler optimization where it would
dereference the pud or pmd pointer more than once.  For this same reason
(multiple accesses), I believe the READ_ONCE was added in commit 688272809.

I am no expert in this area, so corrections/comments appreciated.

BTW, I still think there may be races present in lookup_address_in_pgd().
Multiple dereferences of a p4d, pud and pmd are done.
-- 
Mike Kravetz