Re: [External] Re: [PATCH v2 3/6] mm: hugetlb: fix a race between freeing and dissolving the page

[Michal Hocko <mhocko@suse.com>]

On Fri 08-01-21 19:52:54, Muchun Song wrote:
> On Fri, Jan 8, 2021 at 7:44 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Fri 08-01-21 18:08:57, Muchun Song wrote:
> > > On Fri, Jan 8, 2021 at 5:31 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Fri 08-01-21 17:01:03, Muchun Song wrote:
> > > > > On Fri, Jan 8, 2021 at 4:43 PM Michal Hocko <mhocko@suse.com> wrote:
> > > > > >
> > > > > > On Thu 07-01-21 23:11:22, Muchun Song wrote:
> > > > [..]
> > > > > > > But I find a tricky problem to solve. See free_huge_page().
> > > > > > > If we are in non-task context, we should schedule a work
> > > > > > > to free the page. We reuse the page->mapping. If the page
> > > > > > > is already freed by the dissolve path. We should not touch
> > > > > > > the page->mapping. So we need to check PageHuge().
> > > > > > > The check and llist_add() should be protected by
> > > > > > > hugetlb_lock. But we cannot do that. Right? If dissolve
> > > > > > > happens after it is linked to the list. We also should
> > > > > > > remove it from the list (hpage_freelist). It seems to make
> > > > > > > the thing more complex.
> > > > > >
> > > > > > I am not sure I follow you here but yes PageHuge under hugetlb_lock
> > > > > > should be the reliable way to check for the race. I am not sure why we
> > > > > > really need to care about mapping or other state.
> > > > >
> > > > > CPU0:                               CPU1:
> > > > > free_huge_page(page)
> > > > >   if (PageHuge(page))
> > > > >                                     dissolve_free_huge_page(page)
> > > > >                                       spin_lock(&hugetlb_lock)
> > > > >                                       update_and_free_page(page)
> > > > >                                       spin_unlock(&hugetlb_lock)
> > > > >     llist_add(page->mapping)
> > > > >     // the mapping is corrupted
> > > > >
> > > > > The PageHuge(page) and llist_add() should be protected by
> > > > > hugetlb_lock. Right? If so, we cannot hold hugetlb_lock
> > > > > in free_huge_page() path.
> > > >
> > > > OK, I see. I completely forgot about this snowflake. I thought that
> > > > free_huge_page was a typo missing initial __. Anyway you are right that
> > > > this path needs a check as well. But I don't see why we couldn't use the
> > > > lock here. The lock can be held only inside the !in_task branch.
> > >
> > > Because we hold the hugetlb_lock without disable irq. So if an interrupt
> > > occurs after we hold the lock. And we also free a HugeTLB page. Then
> > > it leads to deadlock.
> >
> > There is nothing really to prevent making hugetlb_lock irq safe, isn't
> > it?
> 
> Yeah. We can make the hugetlb_lock irq safe. But why have we not
> done this? Maybe the commit changelog can provide more information.
> 
> See https://github.com/torvalds/linux/commit/c77c0a8ac4c522638a8242fcb9de9496e3cdbb2d

Dang! Maybe it is the time to finally stack one workaround on top of the
other and put this code into the shape. The amount of hackery and subtle
details has just grown beyond healthy!
-- 
Michal Hocko
SUSE Labs