| Message ID | 20231012011106.2425309-1-hyesoo.yu@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: page_alloc: check the order of compound page event when the order is 0 | expand |
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.
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.
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.
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.
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++) {
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(-)