Re: [PATCH v3 00/10] Move uid filtering to BPF filters

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Jun 02, 2025 at 11:26:12PM -0700, Ian Rogers wrote:
> On Mon, Jun 2, 2025 at 9:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Tue, May 27, 2025 at 01:39:21PM -0700, Ian Rogers wrote:
> > > On Fri, Apr 25, 2025 at 2:40 PM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > Rather than scanning /proc and skipping PIDs based on their UIDs, use
> > > > BPF filters for uid filtering. The /proc scanning in thread_map is
> > > > racy as the PID may exit before the perf_event_open causing perf to
> > > > abort. BPF UID filters are more robust as they avoid the race. The
> > > > /proc scanning also misses processes starting after the perf
> > > > command. Add a helper for commands that support UID filtering and wire
> > > > up. Remove the non-BPF UID filtering support given it doesn't work.
> > > >
> > > > v3: Add lengthier commit messages as requested by Arnaldo. Rebase on
> > > >     tmp.perf-tools-next.
> > > >
> > > > v2: Add a perf record uid test (Namhyung) and force setting
> > > >     system-wide for perf trace and perf record (Namhyung). Ensure the
> > > >     uid filter isn't set on tracepoint evsels.
> > > >
> > > > v1: https://lore.kernel.org/lkml/20250111190143.1029906-1-irogers@google.com/
> > >
> > > Ping. Thanks,
> >
> > I'm ok with preferring BPF over /proc scanning, but still hesitate to
> > remove it since some people don't use BPF.  Can you please drop that
> > part and make parse_uid_filter() conditional on BPF?
> 
> Hi Namhyung,
> 
> The approach of scanning /proc fails as:
> 1) processes that start after perf starts will be missed,
> 2) processes that terminate between being scanned in /proc and
> perf_event_open will cause perf to fail (essentially the -u option is
> just sugar to scan /proc and then provide the processes as if they
> were a -p option - such an approach doesn't need building into the
> tool).

Yeah, I remember we had this discussion before.  I think (1) is not true
as perf events will be inherited to children (but there is a race).  And
(2) is a real problem but it's also about a race and it can succeed.

Maybe we could change it to skip failed events when the target is a
user but that's not the direction you want.

> 
> This patch series adds a test [1] and perf test has lots of processes
> starting and exiting, matching condition (2) above*. If this series
> were changed to an approach that uses BPF and falls back on /proc
> scanning then the -u option would be broken for both reasons above but
> also prove a constant source of test flakes.
> 
> Rather than give the users something both frustrating to use (keeps
> quitting due to failed opens) and broken (missing processes) I think
> it is better to quit perf at that point informing the user they need
> more permissions to load the BPF program. This also makes the -u
> option testable.
> 
> So the request for a change I don't think is sensible as it provides a
> worse user and testing experience. There is also the cognitive load of
> having the /proc scanning code in the code base, whereas the BPF
> filter is largely isolated.

But I think the problem is that it has different requirements - BPF and
root privilege.  So it should be used after checking the requirements
and fail or fallback.

Does it print proper error messages if not?  With that we can deprecate
the existing behavior and remove it later.

Thanks,
Namhyung