Re: [PATCH 0/8] mm/page_owner: Add PID/TGID/COMM and cgroup filtering

["zhen.ni" <zhen.ni@easystack.cn>]

在 2026/8/29 02:36, Andrew Morton 写道:
> On Fri, 28 Aug 2026 08:03:38 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
> 
>> On Fri, Aug 28, 2026 at 11:13:31AM +0800, Zhen Ni wrote:
>>> This patch series adds process and memory cgroup filtering support to
>>> page_owner. Following the previous series that introduced print_mode and
>>> NUMA node filters:
>>>    https://lore.kernel.org/linux-mm/20260707115411.1714314-1-zhen.ni@easystack.cn/
>>>
>>> This series adds filtering capabilities to page_owner, allowing users to
>>> filter output by specific processes and memory cgroups. Users can now
>>> filter page_owner output by PID, TGID, COMM (with wildcard support), and
>>> memory cgroup path. This makes page_owner debugging more focused and
>>> efficient for tracking memory allocations in specific contexts.
>>
>> The majority of this cover letter feels like it should have been
>> documentation put somewhere :)
> 
> yes please.
> 
> The only thing longer than the cover letter is the Sashiko report ;)
> 
> 	https://sashiko.dev/#/patchset/20260828031339.1270699-1-zhen.ni@easystack.cn
> 
> 

Hi Andrew, Lorenzo,

Thanks for the review.

I have analyzed all the Sashiko report findings. Some will be fixed in
the next version, and for the rest I propose not to fix them, with
reasons below. If there are no objections I will send v2 accordingly.

Will be fixed in the next version:

- mm/page_owner.c: drop the kstrdup() copy in parse_pid_t_list(); the
   token will be parsed in place. This also fixes a leak on the success
   path and a kfree() of an advanced (interior) pointer on the error
   path. cmp_int() will replace plain subtraction in cmp_pid_t(), and
   pid values exceeding PID_MAX_LIMIT will be rejected.
- mm/page_owner.c: PAGE_OWNER will select GLOB so glob_match() is
   always linked in; parse_comm_list() will drop its kstrdup() copy the
   same way.
- mm/page_owner.c: the cgroup path buffer will be allocated once per
   read() outside the page_ext RCU read-side critical section instead of
   per page inside get_page_memcg_info() (GFP_KERNEL allocations must
   not sleep there). The memcg= parsing branch will be guarded by
   CONFIG_MEMCG so kernels built without memcg reject the command
   instead of silently enabling a filter that never matches.
- tools/mm/page_owner_filter.c: user-visible input errors (empty
   -p/-t/-c/-g arguments) will print error messages instead of exiting
   silently.
- Documentation: the wildcard pattern in the -c example will be quoted
   to prevent shell glob expansion.

Proposed not to fix, by design:

1. Shared-fd concurrent read/write races (READ_ONCE around the pid
    passed to bsearch, torn reads of pid/tgid lists, glob_match() racing
    comm rewrites, concurrent write() leading to state->memcg_path
    double-allocation): multi-threaded sharing of one page_owner fd is
    not a designed use of this interface. The filters are per-fd state
    meant to be configured once and then read, which is what the
    page_owner_filter tool does. Adding locking to the read path would
    put overhead into the per-page scan loop for no designed benefit.
    This matches the semantics of the original filter introduction.

2. No "clear filter" support (empty pid=/tgid= lists partially clearing
    proc filters, empty memcg=/nid= being rejected): write commands are
    incremental -- "keep the unmentioned filters" -- and there is no
    clear operation by design. To start over, close the fd and open a
    fresh one; the page_owner_filter tool already works this way. This
    also matches the semantics of the original filter introduction.

3. kcalloc() vs kmalloc_array() for new_comm_list: no consumer of the
    list reads past strscpy()'s NUL terminator, so uninitialized bytes
    are unreachable.

4. char cgroup_path[512] in validate_cgroup_path(): acknowledged that
    the kernel side accepts paths up to PATH_MAX (4096). The userspace
    check truncates an over-long path with snprintf() and then fails
    access(), so it is rejected, never silently accepted. Bumping the
    buffer to PATH_MAX would only serve pathological paths; realistic
    cgroup paths are well under 100 bytes, so 512 wastes nothing in
    practice.

If this plan looks reasonable I will send v2.

Thanks,
Zhen Ni