[PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[Hyesoo Yu]

For compound pages, the head sets the PG_head flag and
the tail sets the compound_head to indicate the head page.
If a user allocates a compound page and frees it with a different
order, the compound page information will not be properly
initialized. To detect this problem, compound_page(page) and
the order are compared, but it is not checked when the order is 0.
That error should be checked regardless of the order.

Signed-off-by: Hyesoo Yu <hyesoo.yu@samsung.com>
---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 95546f376302..fc92ac93c7c8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1078,6 +1078,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
 	int bad = 0;
 	bool skip_kasan_poison = should_skip_kasan_poison(page, fpi_flags);
 	bool init = want_init_on_free();
+	bool compound = PageCompound(page);
 
 	VM_BUG_ON_PAGE(PageTail(page), page);
 
@@ -1096,16 +1097,15 @@ static __always_inline bool free_pages_prepare(struct page *page,
 		return false;
 	}
 
+	VM_BUG_ON_PAGE(compound && compound_order(page) != order, page);
+
 	/*
 	 * Check tail pages before head page information is cleared to
 	 * avoid checking PageCompound for order-0 pages.
 	 */
 	if (unlikely(order)) {
-		bool compound = PageCompound(page);
 		int i;
 
-		VM_BUG_ON_PAGE(compound && compound_order(page) != order, page);
-
 		if (compound)
 			page[1].flags &= ~PAGE_FLAGS_SECOND;
 		for (i = 1; i < (1 << order); i++) {
-- 
2.25.1

Re: [PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[Vishal Moola]

On Thu, Oct 12, 2023 at 10:11:06AM +0900, Hyesoo Yu wrote:
> For compound pages, the head sets the PG_head flag and
> the tail sets the compound_head to indicate the head page.
> If a user allocates a compound page and frees it with a different
> order, the compound page information will not be properly
> initialized. To detect this problem, compound_page(page) and
> the order are compared, but it is not checked when the order is 0.
> That error should be checked regardless of the order.

I believe all compound pages are order >= 1, so this error can't occur
when the order is 0.

Re: [PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

On Fri, Oct 13, 2023 at 01:54:08PM -0700, Vishal Moola wrote:
> On Thu, Oct 12, 2023 at 10:11:06AM +0900, Hyesoo Yu wrote:
> > For compound pages, the head sets the PG_head flag and
> > the tail sets the compound_head to indicate the head page.
> > If a user allocates a compound page and frees it with a different
> > order, the compound page information will not be properly
> > initialized. To detect this problem, compound_page(page) and
> > the order are compared, but it is not checked when the order is 0.
> > That error should be checked regardless of the order.
> 
> I believe all compound pages are order >= 1, so this error can't occur
> when the order is 0.
> 

Yes. All compound pages are order >= 1.
However if the user uses the API incorrectly, the order value could be zero.

For example,

addr = alloc_pages(GFP_COMP, 2);
free_pages(addr, 0);

(struct page[16])0xFFFFFFFE21715100 = (
(flags = 0x4000000000000200, lru = (next = 0x0, prev = 0xDEAD000000000122),//  Clear PG_head
(flags = 0x4000000000000000, lru = (next = 0xFFFFFFFE21715101, prev = 0xFFFFFFFF00000201),  // Remain compound head

It is memory leak, and it also makes system stability problem.
on isolation_single_pageblock, That case makes infinite loops.

for (pfn = start_pfn; pfn < boundary_pfn; ) {
	if (PageCompound(page)) { // page[1] is compound page
		struct page *head = compound_head(page); // page[0]
		unsigned long head_pfn = page_to_pfn(head);
		unsigned long nr_pages = compound_nr(head); // nr_pages is 1 since page[0] is not compound page.

 		if (head_pfn + nr_pages <= boundary_pfn) {
			pfn = head_pfn + nr_pages; // pfn is set as page[1].
			continue;
		}
}

So, I guess, we have to check the incorrect use in free_pages_prepare.

Thanks,
Hyesoo Yu.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[Vishal Moola]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

On Sun, Oct 15, 2023 at 5:42 PM Hyesoo Yu <hyesoo.yu@samsung.com> wrote:
>
> On Fri, Oct 13, 2023 at 01:54:08PM -0700, Vishal Moola wrote:
> > On Thu, Oct 12, 2023 at 10:11:06AM +0900, Hyesoo Yu wrote:
> > > For compound pages, the head sets the PG_head flag and
> > > the tail sets the compound_head to indicate the head page.
> > > If a user allocates a compound page and frees it with a different
> > > order, the compound page information will not be properly
> > > initialized. To detect this problem, compound_page(page) and

s/compound_page/compound_order/

> > > the order are compared, but it is not checked when the order is 0.
> > > That error should be checked regardless of the order.

With this many mentions of "the order", it is easy to misinterpret "the
order"
to be referencing the page order rather than the order of pages we are
trying
to free. I recommend replacing "the order" with "the order argument" or
something similar for clarity.

> > I believe all compound pages are order >= 1, so this error can't occur
> > when the order is 0.
> >
>
> Yes. All compound pages are order >= 1.
> However if the user uses the API incorrectly, the order value could be
zero.

I see, thanks for clarifying that.

With the commit message changes above:
Reviewed-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

> For example,
>
> addr = alloc_pages(GFP_COMP, 2);
> free_pages(addr, 0);
>
> (struct page[16])0xFFFFFFFE21715100 = (
> (flags = 0x4000000000000200, lru = (next = 0x0, prev =
0xDEAD000000000122),//  Clear PG_head
> (flags = 0x4000000000000000, lru = (next = 0xFFFFFFFE21715101, prev =
0xFFFFFFFF00000201),  // Remain compound head
>
> It is memory leak, and it also makes system stability problem.
> on isolation_single_pageblock, That case makes infinite loops.
>
> for (pfn = start_pfn; pfn < boundary_pfn; ) {
>         if (PageCompound(page)) { // page[1] is compound page
>                 struct page *head = compound_head(page); // page[0]
>                 unsigned long head_pfn = page_to_pfn(head);
>                 unsigned long nr_pages = compound_nr(head); // nr_pages
is 1 since page[0] is not compound page.
>
>                 if (head_pfn + nr_pages <= boundary_pfn) {
>                         pfn = head_pfn + nr_pages; // pfn is set as
page[1].
>                         continue;
>                 }
> }
>
> So, I guess, we have to check the incorrect use in free_pages_prepare.
>
> Thanks,
> Hyesoo Yu.

[-- Attachment #2: Type: text/html, Size: 3174 bytes --]

Re: [PATCH] mm: page_alloc: check the order of compound page event when the order is 0

[Hyesoo Yu]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On Sun, Oct 15, 2023 at 08:28:18PM -0700, Vishal Moola wrote:
> On Sun, Oct 15, 2023 at 5:42 PM Hyesoo Yu <hyesoo.yu@samsung.com> wrote:
> >
> > On Fri, Oct 13, 2023 at 01:54:08PM -0700, Vishal Moola wrote:
> > > On Thu, Oct 12, 2023 at 10:11:06AM +0900, Hyesoo Yu wrote:
> > > > For compound pages, the head sets the PG_head flag and
> > > > the tail sets the compound_head to indicate the head page.
> > > > If a user allocates a compound page and frees it with a different
> > > > order, the compound page information will not be properly
> > > > initialized. To detect this problem, compound_page(page) and
> 
> s/compound_page/compound_order/
> 
> > > > the order are compared, but it is not checked when the order is 0.
> > > > That error should be checked regardless of the order.
> 
> With this many mentions of "the order", it is easy to misinterpret "the
> order"
> to be referencing the page order rather than the order of pages we are
> trying
> to free. I recommend replacing "the order" with "the order argument" or
> something similar for clarity.
> 

What a good idea! I'll replace that. Thanks for your comments.

> > > I believe all compound pages are order >= 1, so this error can't occur
> > > when the order is 0.
> > >
> >
> > Yes. All compound pages are order >= 1.
> > However if the user uses the API incorrectly, the order value could be
> zero.
> 
> I see, thanks for clarifying that.
> 
> With the commit message changes above:
> Reviewed-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> 

Okay, Thanks for review.

Regards.
Hyesoo Yu.

> > For example,
> >
> > addr = alloc_pages(GFP_COMP, 2);
> > free_pages(addr, 0);
> >
> > (struct page[16])0xFFFFFFFE21715100 = (
> > (flags = 0x4000000000000200, lru = (next = 0x0, prev =
> 0xDEAD000000000122),//  Clear PG_head
> > (flags = 0x4000000000000000, lru = (next = 0xFFFFFFFE21715101, prev =
> 0xFFFFFFFF00000201),  // Remain compound head
> >
> > It is memory leak, and it also makes system stability problem.
> > on isolation_single_pageblock, That case makes infinite loops.
> >
> > for (pfn = start_pfn; pfn < boundary_pfn; ) {
> >         if (PageCompound(page)) { // page[1] is compound page
> >                 struct page *head = compound_head(page); // page[0]
> >                 unsigned long head_pfn = page_to_pfn(head);
> >                 unsigned long nr_pages = compound_nr(head); // nr_pages
> is 1 since page[0] is not compound page.
> >
> >                 if (head_pfn + nr_pages <= boundary_pfn) {
> >                         pfn = head_pfn + nr_pages; // pfn is set as
> page[1].
> >                         continue;
> >                 }
> > }
> >
> > So, I guess, we have to check the incorrect use in free_pages_prepare.
> >
> > Thanks,
> > Hyesoo Yu.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]