On Sun, Oct 15, 2023 at 5:42 PM Hyesoo Yu <hyesoo.yu@xxxxxxxxxxx> 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"
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@xxxxxxxxx>
> 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.
