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

[Baolin Wang]

On 8/24/2022 7:55 PM, David Hildenbrand wrote:
> On 24.08.22 11:41, Baolin Wang wrote:
> > On 8/24/2022 3:31 PM, David Hildenbrand wrote:
> > > > > > > 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).
> >
> > OK, fair enough.
> >
> > > > 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
> >
> > Thanks.
> >
> > > > 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.
> >
> > I think this is a real problem (not theoretical), and easy to write some
> > code to show the issue. For example, suppose thread A is trying to look
> > up a CONT-PTE size hugetlb page under the lock, however antoher thread B
> > can migrate the CONT-PTE hugetlb page at the same time, which will cause
> > thread A to get an incorrect page, if thread A want to do something for
> > this incorrect page, error occurs.
> >
> > Actually we also want to backport these fixes to the distro with old
> > kernel versions to make the hugetlb more stable. Otherwise we must hit
> > these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.
> >
> > Anyway, if you and Mike still think these issues are not important
> > enough to be fixed in the old versions, I can do the cleanup firstly.
>
> [asking myself which follow_page() users actually care about hugetlb,
> and why we need this handling in follow_page at all]
>
> Which follow_page() user do we care about here? Primarily mm/migrate.c
> only I assume?

Right, mainly affects the move_pages() syscall I think. Yes, I can not 
know all of the users of the move_pages() syscall now or in the future 
in our data center, but like I said the move_pages() syscall + hugetlb 
can be a real potential stability issue.