The uprobe_write_opcode function currently updates also refctr offset
if there's one defined for uprobe.
This is not handy for following changes which needs to make several
updates (writes) to install or remove uprobe, but update refctr offset
just once.
Adding set_swbp_refctr/set_orig_refctr which makes sure refctr offset
is updated.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/uprobes.h | 2 +-
kernel/events/uprobes.c | 62 ++++++++++++++++++++++++-----------------
2 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 7447e15559b8..d3496f7bc583 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -194,7 +194,7 @@ extern bool is_swbp_insn(uprobe_opcode_t *insn);
extern bool is_trap_insn(uprobe_opcode_t *insn);
extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
-extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma, unsigned long vaddr, uprobe_opcode_t);
+extern int uprobe_write_opcode(struct vm_area_struct *vma, unsigned long vaddr, uprobe_opcode_t opcode);
extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool);
extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 87bca004ee6a..8b31340ed1c3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -486,13 +486,12 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
* Called with mm->mmap_lock held for read or write.
* Return 0 (success) or a negative errno.
*/
-int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
- const unsigned long opcode_vaddr, uprobe_opcode_t opcode)
+int uprobe_write_opcode(struct vm_area_struct *vma, const unsigned long opcode_vaddr,
+ uprobe_opcode_t opcode)
{
const unsigned long vaddr = opcode_vaddr & PAGE_MASK;
struct mm_struct *mm = vma->vm_mm;
- struct uprobe *uprobe;
- int ret, is_register, ref_ctr_updated = 0;
+ int ret, is_register;
unsigned int gup_flags = FOLL_FORCE;
struct mmu_notifier_range range;
struct folio_walk fw;
@@ -500,7 +499,6 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
struct page *page;
is_register = is_swbp_insn(&opcode);
- uprobe = container_of(auprobe, struct uprobe, arch);
if (WARN_ON_ONCE(!is_cow_mapping(vma->vm_flags)))
return -EINVAL;
@@ -528,17 +526,6 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
goto out;
}
- /* We are going to replace instruction, update ref_ctr. */
- if (!ref_ctr_updated && uprobe->ref_ctr_offset) {
- ret = update_ref_ctr(uprobe, mm, is_register ? 1 : -1);
- if (ret) {
- folio_put(folio);
- goto out;
- }
-
- ref_ctr_updated = 1;
- }
-
ret = 0;
if (unlikely(!folio_test_anon(folio))) {
VM_WARN_ON_ONCE(is_register);
@@ -580,10 +567,6 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
}
out:
- /* Revert back reference counter if instruction update failed. */
- if (ret < 0 && is_register && ref_ctr_updated)
- update_ref_ctr(uprobe, mm, -1);
-
/* try collapse pmd for compound page */
if (ret > 0)
collapse_pte_mapped_thp(mm, vaddr, false);
@@ -603,7 +586,27 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
int __weak set_swbp(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
unsigned long vaddr)
{
- return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN);
+ return uprobe_write_opcode(vma, vaddr, UPROBE_SWBP_INSN);
+}
+
+static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
+{
+ struct mm_struct *mm = vma->vm_mm;
+ int err;
+
+ /* We are going to replace instruction, update ref_ctr. */
+ if (uprobe->ref_ctr_offset) {
+ err = update_ref_ctr(uprobe, mm, 1);
+ if (err)
+ return err;
+ }
+
+ err = set_swbp(&uprobe->arch, vma, vaddr);
+
+ /* Revert back reference counter if instruction update failed. */
+ if (err && uprobe->ref_ctr_offset)
+ update_ref_ctr(uprobe, mm, -1);
+ return err;
}
/**
@@ -618,8 +621,17 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
int __weak set_orig_insn(struct arch_uprobe *auprobe,
struct vm_area_struct *vma, unsigned long vaddr)
{
- return uprobe_write_opcode(auprobe, vma, vaddr,
- *(uprobe_opcode_t *)&auprobe->insn);
+ return uprobe_write_opcode(vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn);
+}
+
+static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
+{
+ int err = set_orig_insn(&uprobe->arch, vma, vaddr);
+
+ /* Revert back reference counter even if instruction update failed. */
+ if (uprobe->ref_ctr_offset)
+ update_ref_ctr(uprobe, vma->vm_mm, -1);
+ return err;
}
/* uprobe should have guaranteed positive refcount */
@@ -1158,7 +1170,7 @@ static int install_breakpoint(struct uprobe *uprobe, struct vm_area_struct *vma,
if (first_uprobe)
set_bit(MMF_HAS_UPROBES, &mm->flags);
- ret = set_swbp(&uprobe->arch, vma, vaddr);
+ ret = set_swbp_refctr(uprobe, vma, vaddr);
if (!ret)
clear_bit(MMF_RECALC_UPROBES, &mm->flags);
else if (first_uprobe)
@@ -1173,7 +1185,7 @@ static int remove_breakpoint(struct uprobe *uprobe, struct vm_area_struct *vma,
struct mm_struct *mm = vma->vm_mm;
set_bit(MMF_RECALC_UPROBES, &mm->flags);
- return set_orig_insn(&uprobe->arch, vma, vaddr);
+ return set_orig_refctr(uprobe, vma, vaddr);
}
struct map_info {
--
2.49.0On 04/21, Jiri Olsa wrote:
>
> +static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> +{
> + struct mm_struct *mm = vma->vm_mm;
> + int err;
> +
> + /* We are going to replace instruction, update ref_ctr. */
> + if (uprobe->ref_ctr_offset) {
> + err = update_ref_ctr(uprobe, mm, 1);
> + if (err)
> + return err;
> + }
> +
> + err = set_swbp(&uprobe->arch, vma, vaddr);
> +
> + /* Revert back reference counter if instruction update failed. */
> + if (err && uprobe->ref_ctr_offset)
> + update_ref_ctr(uprobe, mm, -1);
> + return err;
> }
...
> +static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> +{
> + int err = set_orig_insn(&uprobe->arch, vma, vaddr);
> +
> + /* Revert back reference counter even if instruction update failed. */
> + if (uprobe->ref_ctr_offset)
> + update_ref_ctr(uprobe, vma->vm_mm, -1);
> + return err;
> }
This doesn't look right even in the simplest case...
To simplify, suppose that uprobe_register() needs to change a single mm/vma
and set_swbp() fails. In this case uprobe_register() calls uprobe_unregister()
which will find the same vma and call set_orig_refctr(). set_orig_insn() will
do nothing. But update_ref_ctr(uprobe, vma->vm_mm, -1) is wrong/unbalanced.
The current code updates ref_ctr after the verify_opcode() check, so it doesn't
have this problem.
-------------------------------------------------------------------------------
OTOH, I think that the current logic is not really correct too,
/* Revert back reference counter if instruction update failed. */
if (ret < 0 && is_register && ref_ctr_updated)
update_ref_ctr(uprobe, mm, -1);
I think that "Revert back reference counter" logic should not depend on
is_register. Otherwise we can have the unbalanced update_ref_ctr(-1) if
uprobe_unregister() fails, then another uprobe_register() comes at the
same address, and after that uprobe_unregister() succeeds.
Oleg.
On Sun, Apr 27, 2025 at 04:13:35PM +0200, Oleg Nesterov wrote:
> On 04/21, Jiri Olsa wrote:
> >
> > +static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> > +{
> > + struct mm_struct *mm = vma->vm_mm;
> > + int err;
> > +
> > + /* We are going to replace instruction, update ref_ctr. */
> > + if (uprobe->ref_ctr_offset) {
> > + err = update_ref_ctr(uprobe, mm, 1);
> > + if (err)
> > + return err;
> > + }
> > +
> > + err = set_swbp(&uprobe->arch, vma, vaddr);
> > +
> > + /* Revert back reference counter if instruction update failed. */
> > + if (err && uprobe->ref_ctr_offset)
> > + update_ref_ctr(uprobe, mm, -1);
> > + return err;
> > }
> ...
> > +static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> > +{
> > + int err = set_orig_insn(&uprobe->arch, vma, vaddr);
> > +
> > + /* Revert back reference counter even if instruction update failed. */
> > + if (uprobe->ref_ctr_offset)
> > + update_ref_ctr(uprobe, vma->vm_mm, -1);
> > + return err;
> > }
>
> This doesn't look right even in the simplest case...
>
> To simplify, suppose that uprobe_register() needs to change a single mm/vma
> and set_swbp() fails. In this case uprobe_register() calls uprobe_unregister()
> which will find the same vma and call set_orig_refctr(). set_orig_insn() will
> do nothing. But update_ref_ctr(uprobe, vma->vm_mm, -1) is wrong/unbalanced.
>
> The current code updates ref_ctr after the verify_opcode() check, so it doesn't
> have this problem.
ah right :-\
could set_swbp/set_orig_insn return > 0 in case the memory was actually updated?
and we would update the refctr based on that, like:
+static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
+{
+ struct mm_struct *mm = vma->vm_mm;
+ int err;
+
+ err = set_swbp(&uprobe->arch, vma, vaddr);
+ if (err > 0 && uprobe->ref_ctr_offset)
+ update_ref_ctr(uprobe, mm, 1);
+ return err;
+}
+static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
+{
+ int err = set_orig_insn(&uprobe->arch, vma, vaddr);
+
+ /* Revert back reference counter even if instruction update failed. */
+ if (err > 0 && uprobe->ref_ctr_offset)
+ update_ref_ctr(uprobe, vma->vm_mm, -1);
+ return err;
+}
but then what if update_ref_ctr fails..
>
> -------------------------------------------------------------------------------
> OTOH, I think that the current logic is not really correct too,
>
> /* Revert back reference counter if instruction update failed. */
> if (ret < 0 && is_register && ref_ctr_updated)
> update_ref_ctr(uprobe, mm, -1);
>
> I think that "Revert back reference counter" logic should not depend on
> is_register. Otherwise we can have the unbalanced update_ref_ctr(-1) if
> uprobe_unregister() fails, then another uprobe_register() comes at the
> same address, and after that uprobe_unregister() succeeds.
sounds good to me
jirka
On Mon, Apr 28, 2025 at 12:51:57PM +0200, Jiri Olsa wrote: > On Sun, Apr 27, 2025 at 04:13:35PM +0200, Oleg Nesterov wrote: SNIP > > > > ------------------------------------------------------------------------------- > > OTOH, I think that the current logic is not really correct too, > > > > /* Revert back reference counter if instruction update failed. */ > > if (ret < 0 && is_register && ref_ctr_updated) > > update_ref_ctr(uprobe, mm, -1); > > > > I think that "Revert back reference counter" logic should not depend on > > is_register. Otherwise we can have the unbalanced update_ref_ctr(-1) if > > uprobe_unregister() fails, then another uprobe_register() comes at the > > same address, and after that uprobe_unregister() succeeds. > > sounds good to me actualy after closer look, I don't see how this code could be triggered in the first place.. any hint on how to hit such case? like: - ref_ctr_offset is updated - we fail somehow - "if (ret < 0 && ref_ctr_updated)" is true on the way out thanks, jirka
I'm on PTO and traveling until May 15 without my working laptop, can't read the code. Quite possibly I am wrong, but let me try to recall what this code does... - So. uprobe_register() succeeds and changes ref_ctr from 0 to 1. - uprobe_unregister() fails but decrements ref_ctr back to zero. Because the "Revert back reference counter if instruction update failed" logic doesn't apply if is_register is true. Since uprobe_unregister() fails, this uprobe won't be removed. IIRC, we even have the warning about that. - another uprobe_register() comes and re-uses the same uprobe. In this case install_breakpoint() will do nothing, ref_ctr won't be updated (right ?) - uprobe_unregister() is called again and this time it succeeds. In this case ref_ctr is changed from 0 to -1. IIRC, we even have some warning for this case. No? Sorry, I can't check my thinking until I return. Oleg. On 05/06, Jiri Olsa wrote: > > On Mon, Apr 28, 2025 at 12:51:57PM +0200, Jiri Olsa wrote: > > On Sun, Apr 27, 2025 at 04:13:35PM +0200, Oleg Nesterov wrote: > > SNIP > > > > > > > ------------------------------------------------------------------------------- > > > OTOH, I think that the current logic is not really correct too, > > > > > > /* Revert back reference counter if instruction update failed. */ > > > if (ret < 0 && is_register && ref_ctr_updated) > > > update_ref_ctr(uprobe, mm, -1); > > > > > > I think that "Revert back reference counter" logic should not depend on > > > is_register. Otherwise we can have the unbalanced update_ref_ctr(-1) if > > > uprobe_unregister() fails, then another uprobe_register() comes at the > > > same address, and after that uprobe_unregister() succeeds. > > > > sounds good to me > > actualy after closer look, I don't see how this code could be triggered > in the first place.. any hint on how to hit such case? like: > > - ref_ctr_offset is updated > > - we fail somehow > > - "if (ret < 0 && ref_ctr_updated)" is true on the way out > > thanks, > jirka >
On Tue, May 06, 2025 at 04:01:45PM +0200, Oleg Nesterov wrote:
> I'm on PTO and traveling until May 15 without my working laptop, can't read
> the code.
>
> Quite possibly I am wrong, but let me try to recall what this code does...
>
> - So. uprobe_register() succeeds and changes ref_ctr from 0 to 1.
>
> - uprobe_unregister() fails but decrements ref_ctr back to zero. Because the
> "Revert back reference counter if instruction update failed" logic doesn't
> apply if is_register is true.
>
> Since uprobe_unregister() fails, this uprobe won't be removed. IIRC, we even
> have the warning about that.
>
> - another uprobe_register() comes and re-uses the same uprobe. In this case
> install_breakpoint() will do nothing, ref_ctr won't be updated (right ?)
right, because int3 is still in place and verify_opcode returns 0
>
> - uprobe_unregister() is called again and this time it succeeds. In this case
> ref_ctr is changed from 0 to -1. IIRC, we even have some warning for this
> case.
AFAICS that should not happen, there's check below in __update_ref_ctr:
if (unlikely(*ptr + d < 0)) {
pr_warn("ref_ctr going negative. vaddr: 0x%lx, "
"curr val: %d, delta: %d\n", vaddr, *ptr, d);
ret = -EINVAL;
goto out;
}
*ptr += d;
ret = 0;
...
but it still prevents the uprobe from 2nd register to trigger,
so I think the change you suggest makes sense
few things first..
- how do you make uprobe_unregister fail after succesful uprobe_register?
I had to instrument the code to do that for me
- I see one extra uprobe_write_opcode call during unregister (check below)
seems it does no harm, but looks strange
current code:
1st register:
- uprobe_register succeeds and changes ref_ctr_offset from 0 to 1
1st unregister:
- first there's uprobe_perf_close -> uprobe_apply call that ends up in
remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail
- followed by __probe_event_disable -> uprobe_unregister_nosync call
that ends up in remove_breakpoint call that will fail to decrement
ref_ctr_offset to -1 (and ref_ctr_offset stays 0) and fail
- uprobe is leaked
2nd register:
- another uprobe_register() comes and re-uses the same uprobe. In this case
install_breakpoint() will do nothing, ref_ctr won't be updated, stays 0
so uprobe WILL NOT trigger
2nd unregister:
- both attempts (from uprobe_perf_close and __probe_event_disable as above)
to write original instruction will fail, because ref_ctr_offset
update fails and uprobe_write_opcode bails out
with the attached change we will do:
1st register:
- uprobe_register succeeds and changes ref_ctr_offset from 0 to 1
1st unregister:
- first there's uprobe_perf_close -> uprobe_apply call that ends up in
remove_breakpoint call that will decrement ref_ctr_offset to 0 and fail
and restore ref_ctr_offset to 1
- followed by __probe_event_disable -> uprobe_unregister_nosync call
that ends up in remove_breakpoint call that will do the same as
previous step, ref_ctr_offset is 1
- uprobe is leaked
2nd register:
- another uprobe_register() comes and re-uses the same uprobe. In this case
install_breakpoint() will do nothing, ref_ctr won't be updated, stays 1,
so uprobe WILL trigger
2nd unregister:
- succeeds, and ref_ctr_offset is 0
jirka
---
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 207432e92386..65bfe52ed729 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -589,8 +589,8 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
out:
/* Revert back reference counter if instruction update failed. */
- if (ret < 0 && is_register && ref_ctr_updated)
- update_ref_ctr(uprobe, mm, -1);
+ if (ret < 0 && ref_ctr_updated)
+ update_ref_ctr(uprobe, mm, is_register ? -1 : 1);
/* try collapse pmd for compound page */
if (ret > 0)
I am still traveling, will actually read your email when I get back...
On 05/09, Jiri Olsa wrote:
>
> On Tue, May 06, 2025 at 04:01:45PM +0200, Oleg Nesterov wrote:
> >
> > - uprobe_unregister() is called again and this time it succeeds. In this case
> > ref_ctr is changed from 0 to -1. IIRC, we even have some warning for this
> > case.
>
> AFAICS that should not happen, there's check below in __update_ref_ctr:
>
> if (unlikely(*ptr + d < 0)) {
> pr_warn("ref_ctr going negative. vaddr: 0x%lx, "
> "curr val: %d, delta: %d\n", vaddr, *ptr, d);
> ret = -EINVAL;
> goto out;
> }
OK,
> few things first..
>
> - how do you make uprobe_unregister fail after succesful uprobe_register?
> I had to instrument the code to do that for me
I guess _unregister() should not fail "in practice" after
get_user_page + verify_opcode, yet I think we should not rely on this, if possible.
But I won't argue if you think we can ignore this "impossible" failures, just
this should be documented. Same for update_ref_ctr(), iirc it should "never"
fail if ref_offset is correct.
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -589,8 +589,8 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
>
> out:
> /* Revert back reference counter if instruction update failed. */
> - if (ret < 0 && is_register && ref_ctr_updated)
> - update_ref_ctr(uprobe, mm, -1);
> + if (ret < 0 && ref_ctr_updated)
> + update_ref_ctr(uprobe, mm, is_register ? -1 : 1);
Yes, this is what I meant.
Oleg.
On Mon, Apr 28, 2025 at 12:51:57PM +0200, Jiri Olsa wrote:
> On Sun, Apr 27, 2025 at 04:13:35PM +0200, Oleg Nesterov wrote:
> > On 04/21, Jiri Olsa wrote:
> > >
> > > +static int set_swbp_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> > > +{
> > > + struct mm_struct *mm = vma->vm_mm;
> > > + int err;
> > > +
> > > + /* We are going to replace instruction, update ref_ctr. */
> > > + if (uprobe->ref_ctr_offset) {
> > > + err = update_ref_ctr(uprobe, mm, 1);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + err = set_swbp(&uprobe->arch, vma, vaddr);
> > > +
> > > + /* Revert back reference counter if instruction update failed. */
> > > + if (err && uprobe->ref_ctr_offset)
> > > + update_ref_ctr(uprobe, mm, -1);
> > > + return err;
> > > }
> > ...
> > > +static int set_orig_refctr(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long vaddr)
> > > +{
> > > + int err = set_orig_insn(&uprobe->arch, vma, vaddr);
> > > +
> > > + /* Revert back reference counter even if instruction update failed. */
> > > + if (uprobe->ref_ctr_offset)
> > > + update_ref_ctr(uprobe, vma->vm_mm, -1);
> > > + return err;
> > > }
> >
> > This doesn't look right even in the simplest case...
> >
> > To simplify, suppose that uprobe_register() needs to change a single mm/vma
> > and set_swbp() fails. In this case uprobe_register() calls uprobe_unregister()
> > which will find the same vma and call set_orig_refctr(). set_orig_insn() will
> > do nothing. But update_ref_ctr(uprobe, vma->vm_mm, -1) is wrong/unbalanced.
> >
> > The current code updates ref_ctr after the verify_opcode() check, so it doesn't
> > have this problem.
>
> ah right :-\
>
> could set_swbp/set_orig_insn return > 0 in case the memory was actually updated?
> and we would update the refctr based on that, like:
ok, I think we need to keep the refcnt update inside write_insn and enable it
through argument, so I can use write_insn from swbp_optimize/swbp_unoptimize
and tell it not to do refcnt update
jirka
On Mon, Apr 21, 2025 at 2:45 PM Jiri Olsa <jolsa@kernel.org> wrote: > > The uprobe_write_opcode function currently updates also refctr offset > if there's one defined for uprobe. > > This is not handy for following changes which needs to make several > updates (writes) to install or remove uprobe, but update refctr offset > just once. > > Adding set_swbp_refctr/set_orig_refctr which makes sure refctr offset > is updated. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/uprobes.h | 2 +- > kernel/events/uprobes.c | 62 ++++++++++++++++++++++++----------------- > 2 files changed, 38 insertions(+), 26 deletions(-) > LGTM Acked-by: Andrii Nakryiko <andrii@kernel.org> [...]
© 2016 - 2025 Red Hat, Inc.