lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fXTLBb1+PghPCraJQcXY518Jtt3AwPLpoXvjXErW79U5w@mail.gmail.com>
Date: Wed, 12 Feb 2025 23:27:38 -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 Wed, Feb 12, 2025 at 5:44 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> 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@...nel.org> 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@...nel.org> 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.

As I keep repeating the '-u' option has never worked in the past, it
either early exited or missed recording new processes.

The documentation for `perf record` is:
```
...
       -a, --all-cpus
          System-wide collection from all CPUs (default if no target
is specified).
...
       -u, --uid=
          Record events in threads owned by uid. Name or number.
...
```
So `-a` is implied without a target/workload but you are specifying a
workload and `-u`. So do you want samples from the workload or from
the user's processes? I can tell from your command line being sleep
that you want it to imply `-a` but would that be true if it were a
benchmark? For the benchmark case you probably don't want `-a`
implied. If you specify `-p` and `-u` should the processes that don't
match -u be sampled or are you expecting implicit system wide
profiling now?

I agree the behavior in the patch series doesn't match the existing
behavior, but I'm not sure the existing behavior agrees with the
documentation nor working as expected. Having the `-u` not select
system wide, as in the patch, seems to agree with the documentation
better. You have specified a target workload/process/eventual pid and
you want samples owned by the uid, why should you now start getting
samples from other processes? With `-p` you wouldn't expect `-a` to
suddenly get implied, I'm not sure it makes any more sense with any
target/workload.

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

I think that is an okay addition to BPF filters in general. I'm wary
of having patch series feature crept into requiring the entire tool
being reimplemented.

Thanks,
Ian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ