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

[Lorenzo Stoakes (ARM)]

On Tue, Sep 01, 2026 at 02:34:34PM +0800, zhen.ni wrote:
>
>
> 在 2026/8/29 02:36, Andrew Morton 写道:
> > On Fri, 28 Aug 2026 08:03:38 +0100 "Lorenzo Stoakes (ARM)" <ljs@xxxxxxxxxx> 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@xxxxxxxxxxxx/
> > > >
> > > > 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@xxxxxxxxxxxx
> >
> >
>
> 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.

Sorry but this kind of 'summary', 'do you agree with the plan' email is not
acceptable.

You have your feedback, reply to people like a human being directly to them,
thank you very much.

At this point, based on past experience, I have to ask if you're using an LLM?
If so please disclose this as per kernel guidelines:

https://docs.kernel.org/process/coding-assistants.html
https://docs.kernel.org/process/generated-content.html

>
> Thanks,
> Zhen Ni

--
Cheers, Lorenzo