Re: [PATCH] tools/mm: rewrite show_page_info.py with modular design and new features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] tools/mm: broken page_type bitfield decoding in _page_type()
- [Medium] tools/mm: incorrect rmap resolution in get_rmap_info() and _rmap_fields()
- [Medium] tools/mm: broken swap cache detection in _page_fields() and _swapcache_info()
- [Medium] tools/mm: unmasked upper PTE bits and non-present PTE decoding in _walk_page_table()
- [Low] tools/mm: wrong slab stride and tail page access in _scan_page_types()
--

commit 16444b1496af4eed07be39cdba3cef7d0140f1da
Author: Ye Liu <liuye@kylinos.cn>

tools/mm: rewrite show_page_info.py with modular design and new features

This commit rewrites tools/mm/show_page_info.py into modular field-collector
functions, adds multiple input modes (--pfn, --page, --user, --selftest), and
introduces rmap, PTE walk, NUMA node/zone, page type, and memcg inspection
features.

> diff --git a/tools/mm/show_page_info.py b/tools/mm/show_page_info.py
> index c46d8ea283d7d..eea1f8b8fa9fc 100644
> --- a/tools/mm/show_page_info.py
> +++ b/tools/mm/show_page_info.py

[ ... ]

> +def _vma_owner(vma, folio_index, page_size):
> +    """Extract (pid, comm, vm_start, vm_end, mm_ptr, vaddr) from a VMA."""
> +    mm_ptr = vma.vm_mm
> +    owner = mm_ptr.read_().owner.read_()
> +    vaddr = vma.vm_start.value_() + \
> +        (folio_index - vma.vm_pgoff.value_()) * page_size
> +    return (owner.pid.value_(), owner.comm.string_().decode(),
> +            vma.vm_start.value_(), vma.vm_end.value_(), mm_ptr, vaddr)
> +
> +
> +def get_rmap_info(page):
> +    """Find processes that map this page via reverse mapping."""
> +    results = []
> +    head = compound_head(page) if PageCompound(page) else page
> +    mapping = head.mapping.value_()
> +    if mapping == 0:
> +        return results
> +
> +    page_size = prog["PAGE_SIZE"].value_()
> +    try:
> +        folio_index = head.__folio_index.value_()
> +    except (AttributeError, FaultError):
> +        folio_index = 0
> +
> +    try:
> +        if mapping & 0x1:
> +            # Anonymous page: walk anon_vma interval tree.
> +            # Note: this returns all VMAs sharing the anon_vma, which may
> +            # include VMAs that don't actually map this page. The PTE walk
> +            # distinguishes actual mappings (shows "not present" otherwise).
> +            anon_vma = Object(prog, "struct anon_vma *", mapping & ~3)
> +            for avc in rbtree_inorder_for_each_entry(
> +                "struct anon_vma_chain", anon_vma.rb_root.rb_root, "rb"):
> +                try:
> +                    results.append(
> +                        _vma_owner(avc.vma.read_(), folio_index, page_size))
> +                except FaultError:
> +                    continue
> +        else:
> +            addr_space = Object(prog, "struct address_space *", mapping & ~3)
> +            for vma in rbtree_inorder_for_each_entry(
> +                "struct vm_area_struct", addr_space.i_mmap.rb_root,
> +                "shared.rb"):
> +                try:
> +                    vma_pgoff = vma.vm_pgoff.value_()
> +                    vma_pages = (vma.vm_end.value_() -
> +                                 vma.vm_start.value_()) >> 12
> +                    if vma_pgoff <= folio_index < vma_pgoff + vma_pages:
> +                        results.append(
> +                            _vma_owner(vma, folio_index, page_size))
> +                except FaultError:
> +                    continue
> +    except (FaultError, AttributeError):
> +        pass
> +
> +    return results

[Severity: Medium]
Are there a few issues in how get_rmap_info(), _vma_owner(), and
_rmap_fields() resolve reverse mappings?

First, for KSM pages, FOLIO_MAPPING_KSM (0x3) sets both FOLIO_MAPPING_ANON
(0x1) and FOLIO_MAPPING_ANON_KSM (0x2) in head.mapping, where mapping & ~3
points to a struct ksm_stable_node rather than a struct anon_vma. Checking
only mapping & 0x1 causes get_rmap_info() to cast a ksm_stable_node pointer
to struct anon_vma * and walk it as an anon_vma rbtree.

Second, in _vma_owner(), vaddr is calculated using vma.vm_pgoff:

    vaddr = vma.vm_start.value_() + \
        (folio_index - vma.vm_pgoff.value_()) * page_size

For anonymous pages, folio->index (__folio_index) is relative to
vma_start_anon_pgoff(vma) ((vma->__vm_anon_pgoff_hi << 32) |
vma->__vm_anon_pgoff_lo), not vma->vm_pgoff. For COW anonymous pages in
MAP_PRIVATE file-backed VMAs (such as writable data/bss segments of ELF
binaries), vma->vm_pgoff is the file offset while folio_index is vaddr >>
PAGE_SHIFT, producing a bogus vaddr outside the VMA. In addition, the
anonymous branch in get_rmap_info() does not check whether folio_index falls
within the VMA's anonymous pgoff range, so for split or adjacent VMAs sharing
an anon_vma, _vma_owner() computes a vaddr outside avc.vma, and vma_find(mm,
vaddr) in _rmap_fields() resolves a different VMA instead of avc.vma.

Third, show_page_state() passes head instead of page to _rmap_fields(head,
fields, ...), and get_rmap_info() uses head.__folio_index without adding the
tail-page offset (page_to_pfn(page) - page_to_pfn(head)). When inspecting a
tail page of a compound page, _rmap_fields() computes vaddr and walks the
page tables for the head page instead of the target subpage, and misses VMAs
that only map a subrange of a large folio starting after head.__folio_index.
Also, the file-backed branch hardcodes >> 12 when computing vma_pages instead
of dividing by page_size.

[ ... ]

> +def _page_type(page):
> +    """Decode page->page_type field via enum pagetype discovery."""
> +    try:
> +        pt = page.page_type.value_()
> +    except AttributeError:
> +        return None
> +    try:
> +        enum_type = prog.constant("PGTY_buddy").type_
> +        for name, value in enum_type.enumerators:
> +            if value == pt:
> +                return name[5:]  # strip PGTY_ prefix
> +    except (LookupError, AttributeError):
> +        pass
> +    return None

[Severity: Medium]
Does _page_type() ever match real typed pages, or does it misclassify normal
mapped pages instead?

In include/linux/page-flags.h, enum pagetype defines 8-bit constants
PGTY_buddy (0xf0) through PGTY_mapcount_underflow (0xff), and FOLIO_TYPE_OPS
stores the type in bits 24..31 of the 32-bit page->page_type field:

    enum pagetype {
        /* 0x00-0x7f are positive numbers, ie mapcount */
        /* Reserve 0x80-0xef for mapcount overflow. */
        PGTY_buddy              = 0xf0,
        ...
        PGTY_mapcount_underflow = 0xff
    };

    static inline bool page_type_has_type(int page_type)
    {
        return page_type < (PGTY_mapcount_underflow << 24);
    }

Because _page_type() compares the unshifted 32-bit pt directly against value
(0xf0..0xff), real typed pages (whose page_type is 0xf0000000..0xf8ffffff,
such as 0xf5000000 for PGTY_slab in the commit message's Tests 5-7) never
match. Conversely, because page->page_type shares a union with atomic_t
_mapcount, any normal user-mapped page whose _mapcount happens to be between
0xf0 and 0xff (240..255) will match value == pt and be falsely reported as a
typed page.

Also, even if pt is shifted right by 24 bits, PGTY_mapcount_underflow (0xff)
is an enumerator in enum pagetype and will match normal unmapped pages where
page_type is 0xffffffff (_mapcount == -1) unless excluded, and _base_fields()
passes page rather than head to _page_type(), whereas tail pages of compound
typed folios have page_type == UINT_MAX.

[ ... ]

> +def _walk_page_table(mm, vaddr):
> +    """Walk 4-level page table. Returns list of (level, entry_pa, entry_val).
> +
> +    Note: only supports x86_64 4-level paging (48-bit VA). Does not support
> +    5-level paging (LA57, 57-bit VA) where an additional P4D level exists.
> +    """
> +    page_mask = ~(prog["PAGE_SIZE"].value_() - 1)
> +    entries = []
> +    try:
> +        def _read(table_va, idx):
> +            return prog.read_word(table_va + idx * 8)
> +
> +        base_va = mm.pgd.value_()
> +
> +        # PGD
> +        pgd_idx = (vaddr >> 39) & 0x1FF
> +        pgd_val = _read(base_va, pgd_idx)
> +        pgd_pa = virt_to_phys(prog, base_va).value_() + pgd_idx * 8
> +        entries.append(("PGD", pgd_pa, pgd_val))
> +        if not (pgd_val & 1):
> +            return entries
> +
> +        # PUD
> +        pud_table_pa = pgd_val & page_mask
> +        pud_idx = (vaddr >> 30) & 0x1FF
> +        pud_val = _read(phys_to_virt(prog, pud_table_pa).value_(), pud_idx)
> +        entries.append(("PUD", pud_table_pa + pud_idx * 8, pud_val))
> +        if not (pud_val & 1):
> +            return entries
> +        if pud_val & (1 << 7):
> +            return entries  # 1GB huge page
> +
> +        # PMD
> +        pmd_table_pa = pud_val & page_mask
> +        pmd_idx = (vaddr >> 21) & 0x1FF
> +        pmd_val = _read(phys_to_virt(prog, pmd_table_pa).value_(), pmd_idx)
> +        entries.append(("PMD", pmd_table_pa + pmd_idx * 8, pmd_val))
> +        if not (pmd_val & 1):
> +            return entries
> +        if pmd_val & (1 << 7):
> +            return entries  # 2MB huge page
> +
> +        # PTE
> +        pte_table_pa = pmd_val & page_mask
> +        pte_idx = (vaddr >> 12) & 0x1FF
> +        pte_val = _read(phys_to_virt(prog, pte_table_pa).value_(), pte_idx)
> +        entries.append(("PTE", pte_table_pa + pte_idx * 8, pte_val))
> +    except FaultError:
> +        pass
> +    return entries
> +
> +
> +def _pte_fields(mm, addr, fields):
> +    """Collect page table entries via page table walk."""
> +    if prog.platform.arch != Architecture.X86_64:
> +        return
> +    entries = _walk_page_table(mm, addr)
> +    for i, (level, pa, val) in enumerate(entries):
> +        if not val:
> +            fields[level] = "not present"
> +            continue
> +        if i == len(entries) - 1:
> +            # Final entry (PTE or huge page): show decoded flags
> +            flags = '|'.join(n for n, m in _PTE_FLAGS if val & m)
> +            fields[level] = f"{hex(pa)} => {hex(val)} ({flags})"
> +        else:
> +            # Intermediate level: just address => value
> +            fields[level] = f"{hex(pa)} => {hex(val)}"

[Severity: Medium]
In _walk_page_table(), won't page_mask = ~(prog["PAGE_SIZE"].value_() - 1)
leave upper bits (such as bit 63 NX or bits 52..62) set in pud_table_pa,
pmd_table_pa, and pte_table_pa?

Because Python integers have arbitrary precision, ~(4096 - 1) evaluates to
-4096, where all bits >= 12 are 1. If an intermediate PGD, PUD, or PMD entry
has NX (bit 63) or software bits set, masking with page_mask retains those
high bits and passes an invalid physical address to phys_to_virt(), raising
FaultError and aborting the page table walk.

Also, in both _pte_fields() and _rmap_fields(), the loop checks if not val:
to detect non-present entries. When an entry is non-zero but does not have
the PRESENT bit (bit 0) set (such as a swap PTE, migration entry, or
PROT_NONE entry), val is non-zero, so the code decodes swap/migration bits
against _PTE_FLAGS as if it were a present hardware PTE.

[ ... ]

> +def _swapcache_info(head):
> +    """Get swap entry value if page is in swap cache."""
> +    # drgn does not provide a PageSwapCache() helper, so we check the
> +    # decoded page flags string for PG_swapcache.
> +    flags_str = str(decode_page_flags(head))
> +    if "PG_swapcache" not in flags_str:
> +        return None
> +    try:
> +        return hex(head.private.value_())
> +    except (AttributeError, FaultError):
> +        return "Unknown"
> +
> +
> +def _page_fields(head, fields):
> +    """Collect non-slab page fields (refcount, mapping, anon/file, etc.)."""
> +    mapping = head.mapping.value_()
> +    fields["Page Mapping"] = hex(mapping)
> +
> +    if mapping == 0:
> +        fields["Page Anon/File"] = "Unmapped"
> +        refcount = head._refcount.counter.value_()
> +        if refcount > 0:
> +            fields["Page Refcount"] = refcount
> +            fields["Page Mapcount"] = head._mapcount.counter.value_()
> +        return
> +
> +    fields["Page Refcount"] = head._refcount.counter.value_()
> +    fields["Page Mapcount"] = head._mapcount.counter.value_()
> +
> +    lru = _lru_type(head)
> +    if lru:
> +        fields["LRU"] = lru
> +
> +    swap = _swapcache_info(head)
> +    if swap:
> +        fields["Swap Entry"] = swap

[Severity: Medium]
Can _page_fields() and _swapcache_info() miss actual swap-cache pages while
misidentifying non-swapbacked file pages as swap-cache pages?

In _page_fields(), returning early when mapping == 0 skips both _lru_type()
and _swapcache_info(). However, shmem folios in the swap cache and unmapped
anonymous swap-cache folios have mapping == NULL (0), so their LRU state and
Swap Entry will never be shown.

Meanwhile, in include/linux/page-flags.h, PG_swapcache is an alias for
PG_owner_priv_1 (the same bit number as PG_checked), and
folio_test_swapcache() requires both folio_test_swapbacked(folio) and
PG_swapcache. Because drgn's decode_page_flags() emits all enum pageflags
names for each set bit, any non-swapbacked filesystem page with
PG_owner_priv_1 / PG_checked set has "PG_swapcache" in decode_page_flags().
Without checking PageSwapBacked(head) in _swapcache_info(), filesystem
head.private values will be reported as Swap Entry.

[ ... ]

> +    # Show VMA and PTE details for first 2 mappings (--pfn/--page mode only)
> +    if not show_detail:
> +        return
> +    detail = min(2, len(rmap))
> +    for i in range(detail):
> +        pid, comm, s, e, mm, vaddr = rmap[i]
> +        prefix = f"Rmap {pid} " if detail > 1 else ""
> +        try:
> +            vma = vma_find(mm, vaddr)
> +            fields[f"{prefix}VMA"] = hex(vma.value_())

[Severity: Medium]
What happens in _rmap_fields() if the first two rmap entries belong to the
same process (same pid)? Since prefix only includes pid, won't the second
iteration overwrite the first entry's keys in fields?

[ ... ]

> +def _scan_page_types():
> +    """Scan all pages and categorize by type. Returns {type: page} dict."""
> +    from drgn.helpers.linux.mm import for_each_page
> +
> +    objexts_flag = prog.constant("MEMCG_DATA_OBJEXTS").value_()
> +    mask = prog.constant('__NR_MEMCG_DATA_FLAGS').value_() - 1
> +    base_stride = prog.type("struct slabobj_ext").size
> +    stride = base_stride * 2 if _slab_obj_ext_has_codetag() else base_stride
> +
> +    found = {}
> +    needed = ["anon", "file", "compound_anon", "compound_file",
> +              "slab_multi", "slab_single", "slab_no_objcg", "uncharged"]
> +
> +    for page in for_each_page(prog):
> +        try:
> +            head = compound_head(page) if PageCompound(page) else page
> +
> +            if PageSlab(head):
> +                if page.memcg_data.read_() & objexts_flag:
> +                    try:
> +                        slab = cast("struct slab *", head)
> +                        if slab.obj_exts_needs_objcg.value_():
> +                            n = _count_distinct_memcg(page, stride, mask)

[Severity: Low]
In _scan_page_types(), shouldn't the PageSlab(head) block use head rather
than page when reading memcg_data and calling _count_distinct_memcg(), and
compute the stride per slab via _slab_obj_ext_stride(slab)?

For tail pages of compound slabs, memcg_data and slab.objects live on head,
not page. Also, computing a single fixed stride outside the loop skips the
slab.obj_exts_in_object check that _slab_obj_ext_stride(slab) performs when
SLAB_OBJ_EXT_IN_OBJ is used.

[ ... ]

> +            if page.memcg_data.read_() == 0:
> +                if "uncharged" not in found:
> +                    found["uncharged"] = page
> +                continue

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-show_page_info_refactor-v1-1-42518e2d7de7@linux.dev?part=1