Message-ID: <aAuqIsKEjcVUU7jn@x1.local>
Date: Fri, 25 Apr 2025 11:28:34 -0400
From: Peter Xu <peterx@...hat.com>
To: Ming Wang <wangming01@...ngson.cn>
Cc: Huacai Chen <chenhuacai@...nel.org>, WANG Xuerui <kernel@...0n.name>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Hongchen Zhang <zhanghongchen@...ngson.cn>,
	loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
	lixuefeng@...ngson.cn, gaojuxin@...ngson.cn, chenhuacai@...ngson.cn
Subject: Re: [PATCH] mm/hugetlb: LoongArch: Return NULL from
 huge_pte_offset() for none PMD

On Fri, Apr 25, 2025 at 10:47:24AM +0800, Ming Wang wrote:
> Hi Peter Xu,

Hi, Ming,

[...]

> You asked why the check involves pte_none() rather than huge_pte_none(), given that LoongArch
> provides the latter which correctly identifies the invalid_pte_table address.
> 
> That's a great question, and the crux seems to be in how the generic code path works. The crash
> originates within smaps_hugetlb_range() after the generic `is_swap_pte(ptent)` macro returns true.
> Looking at the definition of `is_swap_pte()` (in include/linux/mm.h or similar), it typically
> expands to `!pte_present(pte) && !pte_none(pte)`.
> 
> Critically, even though `smaps_hugetlb_range()` deals with HugeTLB entries (often PMDs cast to pte_t),
> the generic `is_swap_pte()` macro itself, when expanded, calls the **generic `pte_none()` macro**, not
> the specialized `huge_pte_none()`.
> 
> LoongArch's generic `pte_none()` macro is defined as:
> `#define pte_none(pte) (!(pte_val(pte) & ~_PAGE_GLOBAL))`
> This definition does *not* check for the `invalid_pte_table` address and thus returns false for it, 
> leading to `is_swap_pte()` incorrectly returning true.
> 
> So, while LoongArch does provide `huge_pte_none()` which *could* correctly identify the state, it's not
> actually invoked in the code path triggered by `is_swap_pte()` within `smaps_hugetlb_range()`.
> 
> This is why modifying `huge_pte_offset()` seems necessary and appropriate at the architecture level.
> By returning NULL when the underlying PMD entry is none (checked using the correct `pmd_none()`, which *does*
> check for invalid_pte_table on LoongArch), we prevent the invalid pointer and its problematic value from reaching
> `smaps_hugetlb_range()` and subsequently fooling the generic `is_swap_pte()` check that uses the generic `pte_none()`.
> 
> Regarding your point about generic page table walkers possibly needing `pte_none()` itself to handle `invalid_pte_table`
> in the future – I understand the concern. That might indeed be a separate, future enhancement needed for LoongArch's
> generic page table support. However, the current patch addresses the immediate crash within the existing hugetlb-specific
> walker (`smaps_hugetlb_range`) by stopping the problematic value at the source (`huge_pte_offset`), which seems like a
> necessary and correct fix for the present issue.
> 
> Does this explanation clarify the interaction between the generic macros and the arch-specific helpers in this context?

I see what's off here - I'm looking at Andrew's latest mm-unstable, which
contains your other fix already:

commit 2f46598ca15065ff7efac3dba466899608bfc659
Author: Ming Wang <wangming01@...ngson.cn>
Date:   Wed Apr 23 09:03:59 2025 +0800

    smaps: fix crash in smaps_hugetlb_range for non-present hugetlb entries

So we're reading different code base..

Looks like the generic mm code used is_swap_pte() in multiple occurances on
hugetlb ptes already. Besides smap code you mentioned, I at least also see
page_vma_mapped_walk -> check_pte also does it.

I'm not sure what's the best to fix this, and if it means is_swap_pte()
should work on hugetlb pte_t's for all archs.  However you're right if
that's the case your current patch can fix all of them by fixing
huge_pte_offset() in loongarch's impl.  So it looks like at least the
simplest.

Acked-by: Peter Xu <peterx@...hat.com>

Maybe you want to ping the other patch to drop that in mm-unstable too, if
that's not your intention to merge.

Thanks,

-- 
Peter Xu

Powered by blists - more mailing lists