linux-kernel - Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z-wr3-eKLv0_ChZ-@fedora>
Date: Tue, 1 Apr 2025 11:09:35 -0700
From: "Vishal Moola (Oracle)" <vishal.moola@...il.com>
To: Zi Yan <ziy@...dia.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, muchun.song@...ux.dev,
	Miaohe Lin <linmiaohe@...wei.com>,
	Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH] mm/compaction: Fix bug in hugetlb handling pathway

On Tue, Apr 01, 2025 at 11:20:48AM -0400, Zi Yan wrote:
> On 31 Mar 2025, at 22:10, Vishal Moola (Oracle) wrote:
> 
> > The compaction code doesn't take references on pages until we're certain
> > we should attempt to handle it.
> >
> > In the hugetlb case, isolate_or_dissolve_huge_page() may return -EBUSY
> > without taking a reference to the folio associated with our pfn. If our
> > folio's refcount drops to 0, compound_nr() becomes unpredictable, making
> > low_pfn and nr_scanned unreliable.
> > The user-visible effect is minimal - this should rarely happen (if ever).
> >
> > Fix this by storing the folio statistics earlier on the stack (just like
> > the THP and Buddy cases).
> >
> > Also revert commit 66fe1cf7f581 ("mm: compaction: use helper compound_nr
> > in isolate_migratepages_block")
> > to make backporting easier.
> >
> > Fixes: 369fa227c219 ("mm: make alloc_contig_range handle free hugetlb pages")
> > Cc: Miaohe Lin <linmiaohe@...wei.com>
> > Cc: Oscar Salvador <osalvador@...e.de>
> > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@...il.com>
> > ---
> >  mm/compaction.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> 
> <snip>
> 
> > @@ -1011,8 +1011,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> >  				 /* Do not report -EBUSY down the chain */
> >  				if (ret == -EBUSY)
> >  					ret = 0;
> > -				low_pfn += compound_nr(page) - 1;
> > -				nr_scanned += compound_nr(page) - 1;
> > +				low_pfn += (1UL << order) - 1;
> > +				nr_scanned += (1UL << order) - 1;
> >  				goto isolate_fail;
> >  			}
> >
> 
> Right after this, there is “low_pfn += folio_nr_pages(folio) - 1” for
> isolated hugetlb. I wonder if that can use order as well. Maybe not,
> since the order is obtained without taking a reference, but folio_nr_pages()
> is called with a reference. They might be different.

I thought about that as well. There's a very unlikely case where they
could be different: We had a hugetlb page, free'd it, then allocated it
to another hugetlb page. At this point (the success path) we would want
the rest of the compaction counters all the sync up to whichever folio
we ended up isolating anyways.

> Anyway, Reviewed-by: Zi Yan <ziy@...dia.com>

Thanks!

> 
> Best Regards,
> Yan, Zi