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

[Namhyung Kim]

On Wed, Feb 12, 2025 at 03:17:35PM -0800, Ian Rogers wrote:
> On Wed, Feb 12, 2025 at 2:56 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > On Wed, Feb 12, 2025 at 12:00:42PM -0800, Ian Rogers wrote:
> > > On Wed, Feb 12, 2025 at 10:46 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > It's not completely broken and works sometimes.
> > >
> > > No this is the definition of completely broken. If it only works
> > > sometimes then you can't use it, we can't put a test on it, there is
> > > no point in it. Even when it doesn't fail on perf_event_open, does it
> > > work for processes that start after /proc is scanned? No, it is
> > > completely broken.
> >
> > Ok, we have a different definition for it.  Let's ignore the imaginary
> > users of the broken features.  Have you added a test for this change?
> >
> > Anyway I've tested your change and found some issues:
> >
> > 1. It silently exited when BPF-skel is not built.  Better to put some
> >    messages at least.
> >
> >   $ sudo ./perf record -u $(id -u) -- sleep 1
> >
> > 2. Even with BPF-skel, perf record doesn't work well.  It did something
> >    but failed to get sample data for some reason.
> >
> >   $ sudo ./perf record -u $(id -u) -- sleep 1
> >   [ perf record: Woken up 1 times to write data ]
> >   [ perf record: Captured and wrote 0.045 MB perf.data ]
> >
> >    Oh, I think you now need to pass -a because it now works in
> >    system-wide mode and drops samples for other users.
> 
> This is a pre-existing problem, no?

No, it worked without -a in the past.  Please see my previous reply.
I think -u/--uid is one of the supported target in the perf tool (not
for the system call) and it used to disable system-wide mode if -u is
used at the same time.

Thanks,
Namhyung

> 
> > 3. With BPF-skel, non-root users will see this.
> >
> >   $ ./perf record -u $(id -u) -- sleep 1
> >   cannot get fd for 'filters' map
> >   failed to set filter "BPF" on event cycles:P with 13 (Permission denied)
> >
> >    I think it's confusing and better to tell user that you need to run
> >    'perf record --setup-filter pin' as root first.  But maybe due to the
> >    issue #2, you still need to run it as root.
> >
> > Thanks,
> > Namhyung
> >