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

[Namhyung Kim]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/
> 
> 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