linux-kernel - Re: [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230609112944.fc08936beb29a18f7bfb5ae3@linux-foundation.org>
Date:   Fri, 9 Jun 2023 11:29:44 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Tarun Sahu <tsahu@...ux.ibm.com>
Cc:     linux-mm@...ck.org, muchun.song@...ux.dev, mike.kravetz@...cle.com,
        aneesh.kumar@...ux.ibm.com, willy@...radead.org,
        sidhartha.kumar@...cle.com, gerald.schaefer@...ux.ibm.com,
        linux-kernel@...r.kernel.org, jaypatel@...ux.ibm.com
Subject: Re: [PATCH v3] mm/folio: Avoid special handling for order value 0
 in folio_set_order

On Fri,  9 Jun 2023 21:59:07 +0530 Tarun Sahu <tsahu@...ux.ibm.com> wrote:

> folio_set_order(folio, 0) is used in kernel at two places
> __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio.
> Currently, It is called to clear out the folio->_folio_nr_pages and
> folio->_folio_order.
> 
> For __destroy_compound_gigantic_folio:
> In past, folio_set_order(folio, 0) was needed because page->mapping used
> to overlap with _folio_nr_pages and _folio_order. So if these fields were
> left uncleared during freeing gigantic hugepages, they were causing
> "BUG: bad page state" due to non-zero page->mapping. Now, After
> Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to
> CMA") page->mapping has explicitly been cleared out for tail pages. Also,
> _folio_order and _folio_nr_pages no longer overlaps with page->mapping.
> 
> So, folio_set_order(folio, 0) can be removed from freeing gigantic
> folio path (__destroy_compound_gigantic_folio).

The above appears to be a code cleanup only?

> Another place, folio_set_order(folio, 0) is called inside
> __prep_compound_gigantic_folio during error path. Here,
> folio_set_order(folio, 0) can also be removed if we move
> folio_set_order(folio, order) after for loop.
> 
> The patch also moves _folio_set_head call in __prep_compound_gigantic_folio()
> such that we avoid clearing them in the error path.

And the above also sounds like a code cleanup.

> Also, as Mike pointed out:
> "It would actually be better to move the calls _folio_set_head and
> folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why?
> In the current code, the ref count on the 'head page' is still 1 (or more)
> while those calls are made. So, someone could take a speculative ref on the
> page BEFORE the tail pages are set up."
> 
> This way, folio_set_order(folio, 0) is no more needed. And it will also
> helps removing the confusion of folio order being set to 0 (as _folio_order
> field is part of first tail page).
> 
> Testing: I have run LTP tests, which all passes. and also I have written
> the test in LTP which tests the bug caused by compound_nr and page->mapping
> overlapping.

What bug?  Please describe the end-user visible effects of any bug.

And if a bug is indeed fixed, please let's try to identify a Fixes:
target and let's decide whether a -stable backport is needed.

Thanks.

> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c
> 
> Running on older kernel ( < 5.10-rc7) with the above bug this fails while
> on newer kernel and, also with this patch it passes.
>