linux-kernel - Re: [PATCH RFC 5/6] mm: rmap: weaken the WARN_ON in __folio_add_anon

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]

Date: Sat, 27 Jan 2024 15:41:50 -0800
From: Chris Li <chrisl@...nel.org>
To: Barry Song <21cnbao@...il.com>
Cc: ryan.roberts@....com, akpm@...ux-foundation.org, david@...hat.com, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, mhocko@...e.com, 
	shy828301@...il.com, wangkefeng.wang@...wei.com, willy@...radead.org, 
	xiang@...nel.org, ying.huang@...el.com, yuzhao@...gle.com, surenb@...gle.com, 
	steven.price@....com, Barry Song <v-songbaohua@...o.com>, 
	Chuanhua Han <hanchuanhua@...o.com>
Subject: Re: [PATCH RFC 5/6] mm: rmap: weaken the WARN_ON in __folio_add_anon_rmap()

On Thu, Jan 18, 2024 at 3:12 AM Barry Song <21cnbao@...il.com> wrote:
>
> From: Barry Song <v-songbaohua@...o.com>
>
> In do_swap_page(), while supporting large folio swap-in, we are using the helper
> folio_add_anon_rmap_ptes. This is triggerring a WARN_ON in __folio_add_anon_rmap.
> We can make the warning quiet by two ways
> 1. in do_swap_page, we call folio_add_new_anon_rmap() if we are sure the large
> folio is new allocated one; we call folio_add_anon_rmap_ptes() if we find the
> large folio in swapcache.
> 2. we always call folio_add_anon_rmap_ptes() in do_swap_page but weaken the
> WARN_ON in __folio_add_anon_rmap() by letting the WARN_ON less sensitive.
>
> Option 2 seems to be better for do_swap_page() as it can use unified code for
> all cases.
>
> Signed-off-by: Barry Song <v-songbaohua@...o.com>
> Tested-by: Chuanhua Han <hanchuanhua@...o.com>
> ---
>  mm/rmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index f5d43edad529..469fcfd32317 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1304,7 +1304,10 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio,
>                  * page.
>                  */
>                 VM_WARN_ON_FOLIO(folio_test_large(folio) &&
> -                                level != RMAP_LEVEL_PMD, folio);
> +                                level != RMAP_LEVEL_PMD &&
> +                                (!IS_ALIGNED(address, nr_pages * PAGE_SIZE) ||
Some minor nitpick here.
There are two leading "(" in this and next line. This is the first "("
> +                                (folio_test_swapcache(folio) && !IS_ALIGNED(folio->index, nr_pages)) ||
 Second "("  here.

These two "(" are NOT at the same nested level. They should not have
the same indentation.
On my first glance, I misread the scope of the "||" due to the same
level indentation.
We can do one of the two
1) add more indentation on the second "(" to reflect the nesting level.

> +                                page != &folio->page), folio);

Also moving the folio to the next line, because the multiline
expression is huge and complex. Make it obvious the ending "folio" is
not part of the testing condition.

2) Move the multiline test condition to a checking function. Inside
the function it can return early when the shortcut condition is met.
That will also help the readability of this warning condition.

Chris

>                 __folio_set_anon(folio, vma, address,
>                                  !!(flags & RMAP_EXCLUSIVE));
>         } else if (likely(!folio_test_ksm(folio))) {
> --
> 2.34.1
>
>