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

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: <Z6zslLa8XM1ubwXj@google.com>
Date: Wed, 12 Feb 2025 10:46:44 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
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 09:41:04PM -0800, Ian Rogers wrote:
> 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.

It's not completely broken and works sometimes.  And it seems we have an
issue with BPF sideband events.  But it worked when you ran it as root.

  $ sudo perf record -u $(id -u) --no-bpf-event -- sleep 1
  WARNING: Ignored open failure for pid 404758
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 2.754 MB perf.data (3638 samples) ]

  $ sudo perf report -s sym --stdio -q | head
       1.43%  [k] audit_filter_rules.isra.0
       1.33%  [.] pthread_mutex_lock@@GLIBC_2.2.5
       1.06%  [k] __audit_filter_op
       1.05%  [.] __vdso_clock_gettime
       0.94%  [.] _dbus_type_reader_get_current_type
       0.82%  [.] pthread_mutex_trylock@@GLIBC_2.34
       0.76%  [k] __fdget
       0.72%  [.] _dbus_first_type_in_signature
       0.61%  [.] __GI___pthread_mutex_unlock_usercnt
       0.56%  [k] native_sched_clock
  
Thanks,
Namhyung