| 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
| ||
|
Message-ID: <aMNxyx3RADiosttf@mdev>
Date: Fri, 12 Sep 2025 09:05:15 +0800
From: Jinchao Wang <wangjinchao600@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] x86/hw_breakpoint: Unify breakpoint install/uninstall
On Thu, Sep 11, 2025 at 05:03:45PM +0900, Masami Hiramatsu wrote:
> On Wed, 10 Sep 2025 17:39:34 +0800
> Jinchao Wang <wangjinchao600@...il.com> wrote:
>
> > Consolidate breakpoint management into a single helper function to
> > reduce code duplication. This introduces new static helpers for
> > slot management and debug register manipulation.
> >
> > Also, add `<linux/types.h>` to the header file to fix a build
> > dependency.
>
> Looks good to me. Just some nitpicks.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
>
> >
> > Signed-off-by: Jinchao Wang <wangjinchao600@...il.com>
> > ---
> > arch/x86/include/asm/hw_breakpoint.h | 7 +-
> > arch/x86/kernel/hw_breakpoint.c | 151 ++++++++++++++++-----------
> > 2 files changed, 96 insertions(+), 62 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/hw_breakpoint.h b/arch/x86/include/asm/hw_breakpoint.h
> > index 0bc931cd0698..bd437a30dbf2 100644
> > --- a/arch/x86/include/asm/hw_breakpoint.h
> > +++ b/arch/x86/include/asm/hw_breakpoint.h
> > @@ -3,8 +3,8 @@
> > #define _I386_HW_BREAKPOINT_H
> >
> > #include <uapi/asm/hw_breakpoint.h>
> > -
>
> nit: Why this line is removed?
sharp eye, will restore to its original state.
>
> > #define __ARCH_HW_BREAKPOINT_H
> > +#include <linux/types.h>
> >
> > /*
> > * The name should probably be something dealt in
> > @@ -18,6 +18,11 @@ struct arch_hw_breakpoint {
> > u8 type;
> > };
> >
> > +enum bp_slot_action {
> > + BP_SLOT_ACTION_INSTALL,
> > + BP_SLOT_ACTION_UNINSTALL,
> > +};
> > +
> > #include <linux/kdebug.h>
> > #include <linux/percpu.h>
> > #include <linux/list.h>
> > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> > index b01644c949b2..1736063a82b7 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> > @@ -48,7 +48,6 @@ static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
> > */
> > static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM]);
> >
> > -
>
> Ditto.
There were double-space lines, so delete one.
>
> > static inline unsigned long
> > __encode_dr7(int drnum, unsigned int len, unsigned int type)
> > {
> > @@ -84,54 +83,115 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned *len, unsigned *type)
> > return (dr7 >> (bpnum * DR_ENABLE_SIZE)) & 0x3;
> > }
> >
> > -/*
> > - * Install a perf counter breakpoint.
> > - *
> > - * We seek a free debug address register and use it for this
> > - * breakpoint. Eventually we enable it in the debug control register.
> > - *
> > - * Atomic: we hold the counter->ctx->lock and we only handle variables
> > - * and registers local to this cpu.
> > - */
> > -int arch_install_hw_breakpoint(struct perf_event *bp)
> > +static int manage_bp_slot(struct perf_event *bp, enum bp_slot_action action)
> > {
> > - struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> > - unsigned long *dr7;
> > - int i;
> > + struct perf_event *old_bp;
> > + struct perf_event *new_bp;
> > + int slot;
> > +
> > + switch (action) {
> > + case BP_SLOT_ACTION_INSTALL:
> > + old_bp = NULL;
> > + new_bp = bp;
> > + break;
> > + case BP_SLOT_ACTION_UNINSTALL:
> > + old_bp = bp;
> > + new_bp = NULL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> >
> > lockdep_assert_irqs_disabled();
> >
> > - for (i = 0; i < HBP_NUM; i++) {
> > - struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> > + for (slot = 0; slot < HBP_NUM; slot++) {
> > + struct perf_event **curr = this_cpu_ptr(&bp_per_reg[slot]);
> >
> > - if (!*slot) {
> > - *slot = bp;
> > - break;
> > + if (*curr == old_bp) {
> > + *curr = new_bp;
> > + return slot;
> > }
> > }
> >
> > - if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
> > - return -EBUSY;
> > + if (old_bp) {
> > + WARN_ONCE(1, "Can't find matching breakpoint slot");
> > + return -EINVAL;
> > + }
> >
> > - set_debugreg(info->address, i);
> > - __this_cpu_write(cpu_debugreg[i], info->address);
> > + WARN_ONCE(1, "No free breakpoint slots");
> > + return -EBUSY;
> > +}
> >
> > - dr7 = this_cpu_ptr(&cpu_dr7);
> > - *dr7 |= encode_dr7(i, info->len, info->type);
> > +static void setup_hwbp(struct arch_hw_breakpoint *info, int slot, bool enable)
> > +{
> > + unsigned long dr7;
> > +
> > + set_debugreg(info->address, slot);
> > + __this_cpu_write(cpu_debugreg[slot], info->address);
> > +
> > + dr7 = this_cpu_read(cpu_dr7);
> > + if (enable)
> > + dr7 |= encode_dr7(slot, info->len, info->type);
> > + else
> > + dr7 &= ~__encode_dr7(slot, info->len, info->type);
> >
> > /*
> > - * Ensure we first write cpu_dr7 before we set the DR7 register.
> > - * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> > + * Enabling:
> > + * Ensure we first write cpu_dr7 before we set the DR7 register.
> > + * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> > */
> > + if (enable)
> > + this_cpu_write(cpu_dr7, dr7);
> > +
> > barrier();
> >
> > - set_debugreg(*dr7, 7);
> > + set_debugreg(dr7, 7);
> > +
> > if (info->mask)
> > - amd_set_dr_addr_mask(info->mask, i);
> > + amd_set_dr_addr_mask(enable ? info->mask : 0, slot);
> > +
> > + /*
> > + * Disabling:
> > + * Ensure the write to cpu_dr7 is after we've set the DR7 register.
> > + * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> > + */
> > + if (!enable)
> > + this_cpu_write(cpu_dr7, dr7);
> > +}
> > +
> > +static int arch_manage_bp(struct perf_event *bp, enum bp_slot_action action)
> > +{
> > + struct arch_hw_breakpoint *info;
> > + bool install = true;
> > + int slot;
> > +
> > + if (action == BP_SLOT_ACTION_UNINSTALL)
> > + install = false;
>
> This looks a bit unnecessary.
>
> > +
> > + slot = manage_bp_slot(bp, action);
> > + if (slot < 0)
> > + return slot;
> > +
> > + info = counter_arch_bp(bp);
> > + setup_hwbp(info, slot, install);
>
> since you can do
>
> setup_hwbp(info, slot,
> action != BP_SLOT_ACTION_UNINSTALL);
>
> Thank you,
Thanks, I will follow your advice in the next patch.
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>
--
Jinchao
Powered by blists - more mailing lists