Message-ID: <CAP-5=fU+-4igQomO4Q41=7xo6YWyDdVqJdZd34dcMUS-Ua=N1Q@mail.gmail.com>
Date: Tue, 11 Feb 2025 21:41:04 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Hao Ge <gehao@...inos.cn>, James Clark <james.clark@...aro.org>, 
	Howard Chu <howardchu95@...il.com>, Dominique Martinet <asmadeus@...ewreck.org>, 
	Levi Yun <yeoreum.yun@....com>, Xu Yang <xu.yang_2@....com>, 
	Tengda Wu <wutengda@...weicloud.com>, Yang Jihong <yangjihong1@...wei.com>, 
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 00/10] Move uid filtering to BPF filters

On Tue, Feb 11, 2025 at 5:51 PM Namhyung Kim <namhyung@...nel.org> wrote:
> But you removed non-BPF and non-root (w/o pinning BPF) use cases.

I didn't think this was a hard series to understand. It moves the -u
options of perf top and perf record to using BPF filters. The many
reasons for this I already explained:
https://lore.kernel.org/lkml/CAP-5=fUY83gifsMZA0Q45kiQQbAKp2uJxkuwrwGtHK2hiUFRDA@mail.gmail.com/

Your case is a user that isn't exiting and starting processes and
wants to process themself or some other user they some how have
permissions for? They need to not be starting and exiting processes as
new processes are ignored and exiting processes cause the
perf_event_open to fail. What stops such a user passing the list of
processes they have that aren't starting and exiting as a -p option?

If you try something like:
$ perf top -p $(ps --no-headers -u $USER -o pid | awk '{printf "%s%s",
sep, $1; sep=","}')
this is exactly what you get. Does it work? No, the ps and awk
processes terminating but being in the list of processes causes the
perf_event_open for those pids to fail. This is exactly the same
problem as the -u option that you want to keep for this case. The
approach just doesn't work.

Why not make failing to add a -u option fallback on doing the /proc
scan and pretend the processes are a -p option? So now there's a
silent fallback to a broken behavior. New processes won't be profiled
and the data race between the scan and the perf_event_open will cause
failures with non-obvious causes/solutions - mainly, use sudo to run
as root. I'd say this isn't ideal.

This series fixes an option on two commands so that it works in the
sensible use-case, perf running with privileges trying to filter
samples belonging to a specific user. We can say the patch series
doesn't work for the case you give, I don't think anybody cares about
that case, they can get near identical behavior from -p, the behavior
from -p will be clearer than just having -u doing something similar,
namely failing to open for a process and terminate.

You may hate and ignore every point I make and still want to keep the
existing broken behavior. As I've already tried to make clear, you're
adding to the maintenance burden to everyone working in the code base
as the notion of target is fundamental and because you are insisting
on keeping a broken behavior you are also making it untestable. Given
the -u option doesn't work, I strongly suspect nobody uses it. Do I
worry about this series causing harm to the people who aren't using
the option? No I don't as there is a better opportunity in having an
option that (1) does work and (2) results in a simpler code base.

Thanks,
Ian

Powered by blists - more mailing lists