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

[zhen.ni]

在 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.

Thanks,
Zhen Ni