On Mon, Jul 14, 2025 at 07:13:04PM +0900, Masami Hiramatsu wrote:
SNIP
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index 678fb546f0a7..1ee2e5115955 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -20,6 +20,11 @@ typedef u8 uprobe_opcode_t;
> > #define UPROBE_SWBP_INSN 0xcc
> > #define UPROBE_SWBP_INSN_SIZE 1
> >
> > +enum {
> > + ARCH_UPROBE_FLAG_CAN_OPTIMIZE = 0,
> > + ARCH_UPROBE_FLAG_OPTIMIZE_FAIL = 1,
> > +};
> > +
> > struct uprobe_xol_ops;
> >
> > struct arch_uprobe {
> > @@ -45,6 +50,8 @@ struct arch_uprobe {
> > u8 ilen;
> > } push;
> > };
> > +
> > + unsigned long flags;
> > };
> >
> > struct arch_uprobe_task {
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5eecab712376..b80942768f77 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -18,6 +18,7 @@
> > #include <asm/processor.h>
> > #include <asm/insn.h>
> > #include <asm/mmu_context.h>
> > +#include <asm/nops.h>
> >
> > /* Post-execution fixups. */
> >
> > @@ -702,7 +703,6 @@ static struct uprobe_trampoline *create_uprobe_trampoline(unsigned long vaddr)
> > return tramp;
> > }
> >
> > -__maybe_unused
> > static struct uprobe_trampoline *get_uprobe_trampoline(unsigned long vaddr, bool *new)
> > {
> > struct uprobes_state *state = ¤t->mm->uprobes_state;
> > @@ -874,6 +874,285 @@ static int __init arch_uprobes_init(void)
> >
> > late_initcall(arch_uprobes_init);
> >
> > +enum {
> > + OPT_PART,
> > + OPT_INSN,
> > + UNOPT_INT3,
> > + UNOPT_PART,
> > +};
> > +
> > +struct write_opcode_ctx {
> > + unsigned long base;
> > + int update;
> > +};
> > +
> > +static int is_call_insn(uprobe_opcode_t *insn)
> > +{
> > + return *insn == CALL_INSN_OPCODE;
> > +}
> > +
>
> nit: Maybe we need a comment how to verify it as below, or just say
> "See swbp_optimize/unoptimize() for how it works"
>
> /*
> * verify the old opcode starts from swbp or call before update to new opcode.
> * When optimizing from swbp -> call, write 4 byte oprand (OPT_PART), and write
> * the first opcode (OPT_INSN). Also, in unoptimizing, write the first opcode
> * (UNOPT_INT3) and write the rest bytes (OPT_PART).
> * Thus, the *old* `opcode` byte (not @vaddr[0], but ctx->base[0]) must be
> * INT3 (OPT_PART, OPT_INSN, and UNOPT_PART) or CALL(UNOPT_INT3).
> */
will add the comment in here
>
> > +static int verify_insn(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode,
> > + int nbytes, void *data)
> > +{
> > + struct write_opcode_ctx *ctx = data;
> > + uprobe_opcode_t old_opcode[5];
> > +
> > + uprobe_copy_from_page(page, ctx->base, (uprobe_opcode_t *) &old_opcode, 5);
> > +
> > + switch (ctx->update) {
> > + case OPT_PART:
> > + case OPT_INSN:
> > + if (is_swbp_insn(&old_opcode[0]))
> > + return 1;
> > + break;
> > + case UNOPT_INT3:
> > + if (is_call_insn(&old_opcode[0]))
> > + return 1;
> > + break;
>
> > + case UNOPT_PART:
> > + if (is_swbp_insn(&old_opcode[0]))
> > + return 1;
> > + break;
>
> nit: Can we fold this case to the OPT_PART & OPT_INSN case?
> It seems the same.
sure, thanks
jirka
