Re: [PATCH v2 1/5] mm/hugetlb: fix races when looking up a CONT-PTE size hugetlb page

[David Hildenbrand <david@redhat.com>]

>>>>
>>>> IMHO, these follow_huge_xxx() functions are arch-specified at first and
>>>> were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
>>>> hugetlb: Copy general hugetlb code from x86 to mm"), and now there are
>>>> still some arch-specified follow_huge_xxx() definition, for example:
>>>> ia64: follow_huge_addr
>>>> powerpc: follow_huge_pd
>>>> s390: follow_huge_pud
>>>>
>>>> What I mean is that follow_hugetlb_page() is a common and
>>>> not-arch-specified function, is it suitable to change it to be
>>>> arch-specified?
>>>> And thinking more, can we rename follow_hugetlb_page() as
>>>> hugetlb_page_faultin() and simplify it to only handle the page faults of
>>>> hugetlb like the faultin_page() for normal page? That means we can make
>>>> sure only follow_page_mask() can handle hugetlb.
>>>>
>>
>> Something like that might work, but you still have two page table walkers
>> for hugetlb.  I like David's idea (if I understand it correctly) of
> 
> What I mean is we may change the hugetlb handling like normal page:
> 1) use follow_page_mask() to look up a hugetlb firstly.
> 2) if can not get the hugetlb, then try to page fault by 
> hugetlb_page_faultin().
> 3) if page fault successed, then retry to find hugetlb by 
> follow_page_mask().

That implies putting more hugetlbfs special code into generic GUP,
turning it even more complicated. But of course, it depends on how the
end result looks like. My gut feeling was that hugetlb is better handled
in follow_hugetlb_page() separately (just like we do with a lot of other
page table walkers).

> 
> Just a rough thought, and I need more investigation for my idea and 
> David's idea.
> 
>> using follow_hugetlb_page for both cases.  As noted, it will need to be
>> taught how to not trigger faults in the follow_page_mask case.
> 
> Anyway, I also agree we need some cleanup, and firstly I think we should 
> cleanup these arch-specified follow_huge_xxx() on some architectures 
> which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

> 
> However, considering cleanup may need more investigation and 
> refactoring, now I prefer to make these bug-fix patches of this patchset 
> into mainline firstly, which are suitable to backport to old version to 
> fix potential race issues. Mike and David, how do you think? Could you 
> help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb
corner case (CONT pages), and maybe just making it all use
follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to
backport this to stable trees:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a
problem...” type thing)."


Do we actually have any instance of this being a real (and not a
theoretical) problem? If not, I'd rather clean it all up right away.

-- 
Thanks,

David / dhildenb