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

[Mike Kravetz]

On 3/23/20 9:09 AM, Jason Gunthorpe wrote:
> On Sat, Mar 21, 2020 at 04:38:19PM -0700, Mike Kravetz wrote:
> 
>> Andrew dropped this patch from his tree which caused me to go back and
>> look at the status of this patch/issue.
>>
>> It is pretty obvious that code in the current huge_pte_offset routine
>> is racy.  I checked out the assembly code produced by my compiler and
>> verified that the line,
>>
>> 	if (pud_huge(*pud) || !pud_present(*pud))
>>
>> does actually dereference *pud twice.  So, the value could change between
>> those two dereferences.   Longpeng (Mike) could easlily recreate the issue
>> if he put a delay between the two dereferences.  I believe the only
>> reservations/concerns about the patch below was the use of READ_ONCE().
>> Is that correct?
> 
> I'm looking at a similar situation in pagewalk.c right now with PUD,
> and it is very confusing to see that locks are being held, memory
> accessed without READ_ONCE, but actually it has concurrent writes.
> 
> I think it is valuable to annotate with READ_ONCE when the author
> knows this is an unlocked data access, regardless of what the compiler
> does.
> 
> pagewalk probably has the same racy bug you show here, I'm going to
> send a very similar looking patch to pagewalk hopefully soon.

Thanks Jason.

Unfortunately, I replied to the thread without full context for the discussion
we were having.  The primary objection to this patch was the use of READ_ONCE
in these two instances:

>  	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.

> Also, the remark about pmd_offset() seems accurate. The
> get_user_fast_pages() pattern seems like the correct one to emulate:
> 
>   pud = READ_ONCE(*pudp);
>   if (pud_none(pud)) 
>      ..
>   if (!pud_'is a pmd pointer')
>      ..
>   pmdp = pmd_offset(&pud, address);
>   pmd = READ_ONCE(*pmd);
>   [...]
> 
> Passing &pud in avoids another de-reference of the pudp. Honestly all
> these APIs that take in page table pointers and internally
> de-reference them seem very hard to use correctly when the page table
> access isn't fully locked against write.
> 
> This also relies on 'some kind of locking' to prevent the pmdp from
> becoming freed concurrently while this is running.
> 
> .. also this only works if READ_ONCE() is atomic, ie the pud can't be
> 64 bit on a 32 bit platform. At least pmd has this problem, I haven't
> figured out if pud does??
> 
>> Are there any objections to the patch if the READ_ONCE() calls are removed?
> 
> I think if we know there is no concurrent data access then it makes
> sense to keep the READ_ONCE.
> 
> It looks like at least the p4d read from the pgd is also unlocked here
> as handle_mm_fault() writes to it??

Yes, there is no locking required to call huge_pte_offset().

-- 
Mike Kravetz