| Message ID | 20241211133403.208920-5-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | uprobes: Add support to optimize usdt probes on x86_64 | expand |
On Wed, Dec 11, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding arch_uprobe_verify_opcode function, so we can overload > verification for each architecture in following changes. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/uprobes.h | 5 +++++ > kernel/events/uprobes.c | 19 ++++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index cc723bc48c1d..8843b7f99ed0 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -215,6 +215,11 @@ extern void uprobe_handle_trampoline(struct pt_regs *regs); > extern void *arch_uretprobe_trampoline(unsigned long *psize); > extern unsigned long uprobe_get_trampoline_vaddr(void); > extern void uprobe_copy_from_page(struct page *page, unsigned long vaddr, void *dst, int len); > +extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode); > +extern int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > + int nbytes); > +extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes); > #else /* !CONFIG_UPROBES */ > struct uprobes_state { > }; > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 7c2ecf11a573..8068f91de9e3 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -263,7 +263,13 @@ static void uprobe_copy_to_page(struct page *page, unsigned long vaddr, const vo > kunmap_atomic(kaddr); > } > > -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode) > +__weak bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes) > +{ > + return is_swbp_insn(insn); a bit weird that we ignore nbytes here... should we have nbytes == UPROBE_SWBP_INSN_SIZE check somewhere here or inside is_swbp_insn()? > +} > + > +int uprobe_verify_opcode(struct page *page, unsigned long vaddr, > + uprobe_opcode_t *new_opcode) > { > uprobe_opcode_t old_opcode; > bool is_swbp; > @@ -291,6 +297,13 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t > return 1; > } > > +__weak int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > + int nbytes) > +{ > + return uprobe_verify_opcode(page, vaddr, new_opcode); again, dropping nbytes on the floor here > +} > + > static struct delayed_uprobe * > delayed_uprobe_check(struct uprobe *uprobe, struct mm_struct *mm) > { > @@ -479,7 +492,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > bool orig_page_huge = false; > unsigned int gup_flags = FOLL_FORCE; > > - is_register = is_swbp_insn(insn); > + is_register = arch_uprobe_is_register(insn, nbytes); > uprobe = container_of(auprobe, struct uprobe, arch); > > retry: > @@ -490,7 +503,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > if (IS_ERR(old_page)) > return PTR_ERR(old_page); > > - ret = verify_opcode(old_page, vaddr, insn); > + ret = arch_uprobe_verify_opcode(auprobe, old_page, vaddr, insn, nbytes); > if (ret <= 0) > goto put_old; > > -- > 2.47.0 >
On Thu, Dec 12, 2024 at 04:48:05PM -0800, Andrii Nakryiko wrote: > On Wed, Dec 11, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Adding arch_uprobe_verify_opcode function, so we can overload > > verification for each architecture in following changes. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/linux/uprobes.h | 5 +++++ > > kernel/events/uprobes.c | 19 ++++++++++++++++--- > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > index cc723bc48c1d..8843b7f99ed0 100644 > > --- a/include/linux/uprobes.h > > +++ b/include/linux/uprobes.h > > @@ -215,6 +215,11 @@ extern void uprobe_handle_trampoline(struct pt_regs *regs); > > extern void *arch_uretprobe_trampoline(unsigned long *psize); > > extern unsigned long uprobe_get_trampoline_vaddr(void); > > extern void uprobe_copy_from_page(struct page *page, unsigned long vaddr, void *dst, int len); > > +extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode); > > +extern int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > > + int nbytes); > > +extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes); > > #else /* !CONFIG_UPROBES */ > > struct uprobes_state { > > }; > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > index 7c2ecf11a573..8068f91de9e3 100644 > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -263,7 +263,13 @@ static void uprobe_copy_to_page(struct page *page, unsigned long vaddr, const vo > > kunmap_atomic(kaddr); > > } > > > > -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode) > > +__weak bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes) > > +{ > > + return is_swbp_insn(insn); > > a bit weird that we ignore nbytes here... should we have nbytes == > UPROBE_SWBP_INSN_SIZE check somewhere here or inside is_swbp_insn()? the original is_swbp_insn function does not need that and we need nbytes in the overloaded arch_uprobe_is_register to distinguish between 1 byte and 5 byte update.. jirka > > > +} > > + > > +int uprobe_verify_opcode(struct page *page, unsigned long vaddr, > > + uprobe_opcode_t *new_opcode) > > { > > uprobe_opcode_t old_opcode; > > bool is_swbp; > > @@ -291,6 +297,13 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t > > return 1; > > } > > > > +__weak int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > > + int nbytes) > > +{ > > + return uprobe_verify_opcode(page, vaddr, new_opcode); > > again, dropping nbytes on the floor here > > > +} > > + > > static struct delayed_uprobe * > > delayed_uprobe_check(struct uprobe *uprobe, struct mm_struct *mm) > > { > > @@ -479,7 +492,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > bool orig_page_huge = false; > > unsigned int gup_flags = FOLL_FORCE; > > > > - is_register = is_swbp_insn(insn); > > + is_register = arch_uprobe_is_register(insn, nbytes); > > uprobe = container_of(auprobe, struct uprobe, arch); > > > > retry: > > @@ -490,7 +503,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > if (IS_ERR(old_page)) > > return PTR_ERR(old_page); > > > > - ret = verify_opcode(old_page, vaddr, insn); > > + ret = arch_uprobe_verify_opcode(auprobe, old_page, vaddr, insn, nbytes); > > if (ret <= 0) > > goto put_old; > > > > -- > > 2.47.0 > >
On Fri, Dec 13, 2024 at 5:21 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Dec 12, 2024 at 04:48:05PM -0800, Andrii Nakryiko wrote: > > On Wed, Dec 11, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > Adding arch_uprobe_verify_opcode function, so we can overload > > > verification for each architecture in following changes. > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > > include/linux/uprobes.h | 5 +++++ > > > kernel/events/uprobes.c | 19 ++++++++++++++++--- > > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > > index cc723bc48c1d..8843b7f99ed0 100644 > > > --- a/include/linux/uprobes.h > > > +++ b/include/linux/uprobes.h > > > @@ -215,6 +215,11 @@ extern void uprobe_handle_trampoline(struct pt_regs *regs); > > > extern void *arch_uretprobe_trampoline(unsigned long *psize); > > > extern unsigned long uprobe_get_trampoline_vaddr(void); > > > extern void uprobe_copy_from_page(struct page *page, unsigned long vaddr, void *dst, int len); > > > +extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode); > > > +extern int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > > > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > > > + int nbytes); > > > +extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes); > > > #else /* !CONFIG_UPROBES */ > > > struct uprobes_state { > > > }; > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > > index 7c2ecf11a573..8068f91de9e3 100644 > > > --- a/kernel/events/uprobes.c > > > +++ b/kernel/events/uprobes.c > > > @@ -263,7 +263,13 @@ static void uprobe_copy_to_page(struct page *page, unsigned long vaddr, const vo > > > kunmap_atomic(kaddr); > > > } > > > > > > -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode) > > > +__weak bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes) > > > +{ > > > + return is_swbp_insn(insn); > > > > a bit weird that we ignore nbytes here... should we have nbytes == > > UPROBE_SWBP_INSN_SIZE check somewhere here or inside is_swbp_insn()? > > the original is_swbp_insn function does not need that and we need > nbytes in the overloaded arch_uprobe_is_register to distinguish > between 1 byte and 5 byte update.. > and that's my point, if some architecture forgot to override it for nop5 (or similar stuff), this default implementation should reject instruction that's not an original nop, no? > jirka > > > > > > +} > > > + > > > +int uprobe_verify_opcode(struct page *page, unsigned long vaddr, > > > + uprobe_opcode_t *new_opcode) > > > { > > > uprobe_opcode_t old_opcode; > > > bool is_swbp; > > > @@ -291,6 +297,13 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t > > > return 1; > > > } > > > > > > +__weak int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > > > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > > > + int nbytes) > > > +{ > > > + return uprobe_verify_opcode(page, vaddr, new_opcode); > > > > again, dropping nbytes on the floor here > > > > > +} > > > + > > > static struct delayed_uprobe * > > > delayed_uprobe_check(struct uprobe *uprobe, struct mm_struct *mm) > > > { > > > @@ -479,7 +492,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > > bool orig_page_huge = false; > > > unsigned int gup_flags = FOLL_FORCE; > > > > > > - is_register = is_swbp_insn(insn); > > > + is_register = arch_uprobe_is_register(insn, nbytes); > > > uprobe = container_of(auprobe, struct uprobe, arch); > > > > > > retry: > > > @@ -490,7 +503,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, > > > if (IS_ERR(old_page)) > > > return PTR_ERR(old_page); > > > > > > - ret = verify_opcode(old_page, vaddr, insn); > > > + ret = arch_uprobe_verify_opcode(auprobe, old_page, vaddr, insn, nbytes); > > > if (ret <= 0) > > > goto put_old; > > > > > > -- > > > 2.47.0 > > >
On Fri, Dec 13, 2024 at 01:11:30PM -0800, Andrii Nakryiko wrote: > On Fri, Dec 13, 2024 at 5:21 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Thu, Dec 12, 2024 at 04:48:05PM -0800, Andrii Nakryiko wrote: > > > On Wed, Dec 11, 2024 at 5:34 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > > > > > Adding arch_uprobe_verify_opcode function, so we can overload > > > > verification for each architecture in following changes. > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > > > --- > > > > include/linux/uprobes.h | 5 +++++ > > > > kernel/events/uprobes.c | 19 ++++++++++++++++--- > > > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > > > index cc723bc48c1d..8843b7f99ed0 100644 > > > > --- a/include/linux/uprobes.h > > > > +++ b/include/linux/uprobes.h > > > > @@ -215,6 +215,11 @@ extern void uprobe_handle_trampoline(struct pt_regs *regs); > > > > extern void *arch_uretprobe_trampoline(unsigned long *psize); > > > > extern unsigned long uprobe_get_trampoline_vaddr(void); > > > > extern void uprobe_copy_from_page(struct page *page, unsigned long vaddr, void *dst, int len); > > > > +extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode); > > > > +extern int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, > > > > + unsigned long vaddr, uprobe_opcode_t *new_opcode, > > > > + int nbytes); > > > > +extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes); > > > > #else /* !CONFIG_UPROBES */ > > > > struct uprobes_state { > > > > }; > > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > > > index 7c2ecf11a573..8068f91de9e3 100644 > > > > --- a/kernel/events/uprobes.c > > > > +++ b/kernel/events/uprobes.c > > > > @@ -263,7 +263,13 @@ static void uprobe_copy_to_page(struct page *page, unsigned long vaddr, const vo > > > > kunmap_atomic(kaddr); > > > > } > > > > > > > > -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode) > > > > +__weak bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes) > > > > +{ > > > > + return is_swbp_insn(insn); > > > > > > a bit weird that we ignore nbytes here... should we have nbytes == > > > UPROBE_SWBP_INSN_SIZE check somewhere here or inside is_swbp_insn()? > > > > the original is_swbp_insn function does not need that and we need > > nbytes in the overloaded arch_uprobe_is_register to distinguish > > between 1 byte and 5 byte update.. > > > > and that's my point, if some architecture forgot to override it for > nop5 (or similar stuff), this default implementation should reject > instruction that's not an original nop, no? ok, makes sense, will add that thanks, jirka
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index cc723bc48c1d..8843b7f99ed0 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -215,6 +215,11 @@ extern void uprobe_handle_trampoline(struct pt_regs *regs); extern void *arch_uretprobe_trampoline(unsigned long *psize); extern unsigned long uprobe_get_trampoline_vaddr(void); extern void uprobe_copy_from_page(struct page *page, unsigned long vaddr, void *dst, int len); +extern int uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode); +extern int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, + unsigned long vaddr, uprobe_opcode_t *new_opcode, + int nbytes); +extern bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes); #else /* !CONFIG_UPROBES */ struct uprobes_state { }; diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 7c2ecf11a573..8068f91de9e3 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -263,7 +263,13 @@ static void uprobe_copy_to_page(struct page *page, unsigned long vaddr, const vo kunmap_atomic(kaddr); } -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode) +__weak bool arch_uprobe_is_register(uprobe_opcode_t *insn, int nbytes) +{ + return is_swbp_insn(insn); +} + +int uprobe_verify_opcode(struct page *page, unsigned long vaddr, + uprobe_opcode_t *new_opcode) { uprobe_opcode_t old_opcode; bool is_swbp; @@ -291,6 +297,13 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t return 1; } +__weak int arch_uprobe_verify_opcode(struct arch_uprobe *auprobe, struct page *page, + unsigned long vaddr, uprobe_opcode_t *new_opcode, + int nbytes) +{ + return uprobe_verify_opcode(page, vaddr, new_opcode); +} + static struct delayed_uprobe * delayed_uprobe_check(struct uprobe *uprobe, struct mm_struct *mm) { @@ -479,7 +492,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, bool orig_page_huge = false; unsigned int gup_flags = FOLL_FORCE; - is_register = is_swbp_insn(insn); + is_register = arch_uprobe_is_register(insn, nbytes); uprobe = container_of(auprobe, struct uprobe, arch); retry: @@ -490,7 +503,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, if (IS_ERR(old_page)) return PTR_ERR(old_page); - ret = verify_opcode(old_page, vaddr, insn); + ret = arch_uprobe_verify_opcode(auprobe, old_page, vaddr, insn, nbytes); if (ret <= 0) goto put_old;
Adding arch_uprobe_verify_opcode function, so we can overload verification for each architecture in following changes. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/uprobes.h | 5 +++++ kernel/events/uprobes.c | 19 ++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)