linux-kernel - Re: [RFC/PATCH 4/4] perf/x86: Relax privilege filter restriction on AMD IBS

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: <CAM9d7cg-7USE-Ofusc1VDpr3qtviX4xZmoKmBjmEfBDw9XJ+nw@mail.gmail.com>
Date: Mon, 2 Sep 2024 10:30:19 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Stephane Eranian <eranian@...gle.com>, Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [RFC/PATCH 4/4] perf/x86: Relax privilege filter restriction on
 AMD IBS

On Mon, Sep 2, 2024 at 2:12 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Fri, Aug 30, 2024 at 04:29:10PM -0700, Namhyung Kim wrote:
> > While IBS is available for per-thread profiling, still regular users
> > cannot open an event due to the default paranoid setting (2) which
> > doesn't allow unprivileged users to get kernel samples.  That means
> > it needs to set exclude_kernel bit in the attribute but IBS driver
> > would reject it since it has PERF_PMU_CAP_NO_EXCLUDE.  This is not what
> > we want and I've been getting requests to fix this issue.
> >
> > This should be done in the hardware, but until we get the HW fix we may
> > allow exclude_{kernel,user} in the attribute and silently drop the
> > samples in the PMU IRQ handler.  It won't guarantee the sampling
> > frequency or even it'd miss some with fixed period too.  Not ideal,
> > but that'd still be helpful to regular users.
>
> Urgh.... this is really rather bad. And I'm sure a bunch of people are
> going to be spending a lot of time trying to figure out why their
> results don't make sense.

I agree it can be confusing but there are use cases where regular users
want IBS information like memory data source, data address and so on.
Also I realized that software events like cpu-clock use the same logic to
discard samples by privilege mode already.

>
> I realize that having entry hooks to disable/enable the counters is also
> not going to happen, this has a ton of problems too.

Do you mean kernel/user mode change hook?  I guess it'd be too costly.

>
> Also, that PMU passthrough patch set has guest hooks, so you can
> actually do the exclude_host/guest nonsense with those, right?

Oh.. this patch is about exclude_user/kernel not host/guest.  Anyway
it'd be great if IBS could support the guest hooks and allow the exclude
bits.

Thanks,
Namhyung

>
> > This uses perf_exclude_event() which checks regs->cs.  But it should be
> > fine because set_linear_ip() also updates the CS according to the RIP
> > provided by IBS.
> >
> > Cc: Ravi Bangoria <ravi.bangoria@....com>
> > Cc: Stephane Eranian <eranian@...gle.com>
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> >  arch/x86/events/amd/ibs.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> > index e91970b01d62..e40e2255239a 100644
> > --- a/arch/x86/events/amd/ibs.c
> > +++ b/arch/x86/events/amd/ibs.c
> > @@ -290,6 +290,11 @@ static int perf_ibs_init(struct perf_event *event)
> >       if (has_branch_stack(event))
> >               return -EOPNOTSUPP;
> >
> > +     /* handle exclude_{user,kernel} in the IRQ handler */
> > +     if (event->attr.exclude_hv || event->attr.exclude_idle ||
> > +         event->attr.exclude_host || event->attr.exclude_guest)
> > +             return -EINVAL;
> > +
> >       ret = validate_group(event);
> >       if (ret)
> >               return ret;
> > @@ -667,7 +672,6 @@ static struct perf_ibs perf_ibs_fetch = {
> >               .start          = perf_ibs_start,
> >               .stop           = perf_ibs_stop,
> >               .read           = perf_ibs_read,
> > -             .capabilities   = PERF_PMU_CAP_NO_EXCLUDE,
> >       },
> >       .msr                    = MSR_AMD64_IBSFETCHCTL,
> >       .config_mask            = IBS_FETCH_CONFIG_MASK,
> > @@ -691,7 +695,6 @@ static struct perf_ibs perf_ibs_op = {
> >               .start          = perf_ibs_start,
> >               .stop           = perf_ibs_stop,
> >               .read           = perf_ibs_read,
> > -             .capabilities   = PERF_PMU_CAP_NO_EXCLUDE,
> >       },
> >       .msr                    = MSR_AMD64_IBSOPCTL,
> >       .config_mask            = IBS_OP_CONFIG_MASK,
> > @@ -1111,6 +1114,12 @@ static int perf_ibs_handle_irq(struct perf_ibs *perf_ibs, struct pt_regs *iregs)
> >               regs.flags |= PERF_EFLAGS_EXACT;
> >       }
> >
> > +     if (perf_exclude_event(event, &regs)) {
> > +             throttle = perf_event_account_interrupt(event);
> > +             atomic64_inc(&event->dropped_samples);
> > +             goto out;
> > +     }
> > +
> >       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
> >               raw = (struct perf_raw_record){
> >                       .frag = {
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >