1. Patchew
  2. linux
  3. [PATCHv2 perf/core 00/22] uprobes: Add support to optimize usdt probes on x86_64
  4. View patch

[PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock

Jiri Olsa posted 22 patches 7 months, 1 week ago
There is a newer version of this series
[PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Jiri Olsa 7 months, 1 week ago 
Currently unapply_uprobe takes mmap_read_lock, but it might call
remove_breakpoint which eventually changes user pages.

Current code writes either breakpoint or original instruction, so
it can probably go away with that, but with the upcoming change that
writes multiple instructions on the probed address we need to ensure
that any update to mm's pages is exclusive.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/uprobes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 84ee7b590861..257581432cd8 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -483,7 +483,7 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
  * @opcode_vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @opcode_vaddr.
  *
- * Called with mm->mmap_lock held for read or write.
+ * Called with mm->mmap_lock held for write.
  * Return 0 (success) or a negative errno.
  */
 int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
@@ -1464,7 +1464,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
 	struct vm_area_struct *vma;
 	int err = 0;
 
-	mmap_read_lock(mm);
+	mmap_write_lock(mm);
 	for_each_vma(vmi, vma) {
 		unsigned long vaddr;
 		loff_t offset;
@@ -1481,7 +1481,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
 		vaddr = offset_to_vaddr(vma, uprobe->offset);
 		err |= remove_breakpoint(uprobe, vma, vaddr);
 	}
-	mmap_read_unlock(mm);
+	mmap_write_unlock(mm);
 
 	return err;
 }
-- 
2.49.0
Re: [PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Masami Hiramatsu (Google) 7 months ago 
On Thu, 15 May 2025 14:10:58 +0200
Jiri Olsa <jolsa@kernel.org> wrote:

> Currently unapply_uprobe takes mmap_read_lock, but it might call
> remove_breakpoint which eventually changes user pages.
> 
> Current code writes either breakpoint or original instruction, so
> it can probably go away with that, but with the upcoming change that
> writes multiple instructions on the probed address we need to ensure
> that any update to mm's pages is exclusive.
> 

So, this is a bugfix, right?

Thanks,

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/uprobes.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 84ee7b590861..257581432cd8 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -483,7 +483,7 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
>   * @opcode_vaddr: the virtual address to store the opcode.
>   * @opcode: opcode to be written at @opcode_vaddr.
>   *
> - * Called with mm->mmap_lock held for read or write.
> + * Called with mm->mmap_lock held for write.
>   * Return 0 (success) or a negative errno.
>   */
>  int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
> @@ -1464,7 +1464,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
>  	struct vm_area_struct *vma;
>  	int err = 0;
>  
> -	mmap_read_lock(mm);
> +	mmap_write_lock(mm);
>  	for_each_vma(vmi, vma) {
>  		unsigned long vaddr;
>  		loff_t offset;
> @@ -1481,7 +1481,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
>  		vaddr = offset_to_vaddr(vma, uprobe->offset);
>  		err |= remove_breakpoint(uprobe, vma, vaddr);
>  	}
> -	mmap_read_unlock(mm);
> +	mmap_write_unlock(mm);
>  
>  	return err;
>  }
> -- 
> 2.49.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>
Re: [PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Oleg Nesterov 7 months ago 
On 05/20, Masami Hiramatsu wrote:
>
> On Thu, 15 May 2025 14:10:58 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > Currently unapply_uprobe takes mmap_read_lock, but it might call
> > remove_breakpoint which eventually changes user pages.
> >
> > Current code writes either breakpoint or original instruction, so
> > it can probably go away with that, but with the upcoming change that
> > writes multiple instructions on the probed address we need to ensure
> > that any update to mm's pages is exclusive.
> >
>
> So, this is a bugfix, right?

No, mmap_read_lock() is fine.

To remind, this was already discussed with you, see
[PATCH 02/12] uprobes: grab write mmap lock in unapply_uprobe()
https://lore.kernel.org/all/20240625002144.3485799-3-andrii@kernel.org/

And you even reviewed this patch
[PATCH 1/2] uprobes: document the usage of mm->mmap_lock
https://lore.kernel.org/all/20240710140045.GA1084@redhat.com/

But, as the changelog explains, this patch is needed for the upcoming changes.

--------------------------------------------------------------------------
Just in case... I'll try to read this series tomorrow, but at first glance
this version addresses all my concerns.

Oleg.
Re: [PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Masami Hiramatsu (Google) 7 months ago 
On Tue, 20 May 2025 16:19:26 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/20, Masami Hiramatsu wrote:
> >
> > On Thu, 15 May 2025 14:10:58 +0200
> > Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > > Currently unapply_uprobe takes mmap_read_lock, but it might call
> > > remove_breakpoint which eventually changes user pages.
> > >
> > > Current code writes either breakpoint or original instruction, so
> > > it can probably go away with that, but with the upcoming change that
> > > writes multiple instructions on the probed address we need to ensure
> > > that any update to mm's pages is exclusive.
> > >
> >
> > So, this is a bugfix, right?
> 
> No, mmap_read_lock() is fine.
> 
> To remind, this was already discussed with you, see
> [PATCH 02/12] uprobes: grab write mmap lock in unapply_uprobe()
> https://lore.kernel.org/all/20240625002144.3485799-3-andrii@kernel.org/
> 
> And you even reviewed this patch
> [PATCH 1/2] uprobes: document the usage of mm->mmap_lock
> https://lore.kernel.org/all/20240710140045.GA1084@redhat.com/
> 
> But, as the changelog explains, this patch is needed for the upcoming changes.

Oops, OK. So current code is good with either mmap_read_lock() or mmap_write_lock().
But the patch description is a bit confusing. If the point is an atomic (byte?)
update or not, it should describe it.

Thank you,

> 
> --------------------------------------------------------------------------
> Just in case... I'll try to read this series tomorrow, but at first glance
> this version addresses all my concerns.
> 
> Oleg.
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>
Re: [PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Jiri Olsa 6 months, 3 weeks ago 
On Thu, May 22, 2025 at 11:48:22PM +0900, Masami Hiramatsu wrote:
> On Tue, 20 May 2025 16:19:26 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > On 05/20, Masami Hiramatsu wrote:
> > >
> > > On Thu, 15 May 2025 14:10:58 +0200
> > > Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > > Currently unapply_uprobe takes mmap_read_lock, but it might call
> > > > remove_breakpoint which eventually changes user pages.
> > > >
> > > > Current code writes either breakpoint or original instruction, so
> > > > it can probably go away with that, but with the upcoming change that
> > > > writes multiple instructions on the probed address we need to ensure
> > > > that any update to mm's pages is exclusive.
> > > >
> > >
> > > So, this is a bugfix, right?
> > 
> > No, mmap_read_lock() is fine.
> > 
> > To remind, this was already discussed with you, see
> > [PATCH 02/12] uprobes: grab write mmap lock in unapply_uprobe()
> > https://lore.kernel.org/all/20240625002144.3485799-3-andrii@kernel.org/
> > 
> > And you even reviewed this patch
> > [PATCH 1/2] uprobes: document the usage of mm->mmap_lock
> > https://lore.kernel.org/all/20240710140045.GA1084@redhat.com/
> > 
> > But, as the changelog explains, this patch is needed for the upcoming changes.
> 
> Oops, OK. So current code is good with either mmap_read_lock() or mmap_write_lock().
> But the patch description is a bit confusing. If the point is an atomic (byte?)
> update or not, it should describe it.

ok, I'll try to make the changelog more detailed

thanks,
jirka

> 
> Thank you,
> 
> > 
> > --------------------------------------------------------------------------
> > Just in case... I'll try to read this series tomorrow, but at first glance
> > this version addresses all my concerns.
> > 
> > Oleg.
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
Re: [PATCHv2 perf/core 01/22] uprobes: Remove breakpoint in unapply_uprobe under mmap_write_lock
Posted by Jiri Olsa 7 months ago 
On Tue, May 20, 2025 at 08:48:45AM +0900, Masami Hiramatsu wrote:
> On Thu, 15 May 2025 14:10:58 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > Currently unapply_uprobe takes mmap_read_lock, but it might call
> > remove_breakpoint which eventually changes user pages.
> > 
> > Current code writes either breakpoint or original instruction, so
> > it can probably go away with that, but with the upcoming change that
> > writes multiple instructions on the probed address we need to ensure
> > that any update to mm's pages is exclusive.
> > 
> 
> So, this is a bugfix, right?

nope, the current code is fine (I think), but the new code needs to go
through 2 separate instructions changes and we determine the state of
optimization based on the instruction we find, so we need to be sure
there's only one thread inside remove_breakpoint call

jirka

> 
> Thanks,
> 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/events/uprobes.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 84ee7b590861..257581432cd8 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -483,7 +483,7 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
> >   * @opcode_vaddr: the virtual address to store the opcode.
> >   * @opcode: opcode to be written at @opcode_vaddr.
> >   *
> > - * Called with mm->mmap_lock held for read or write.
> > + * Called with mm->mmap_lock held for write.
> >   * Return 0 (success) or a negative errno.
> >   */
> >  int uprobe_write_opcode(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
> > @@ -1464,7 +1464,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
> >  	struct vm_area_struct *vma;
> >  	int err = 0;
> >  
> > -	mmap_read_lock(mm);
> > +	mmap_write_lock(mm);
> >  	for_each_vma(vmi, vma) {
> >  		unsigned long vaddr;
> >  		loff_t offset;
> > @@ -1481,7 +1481,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
> >  		vaddr = offset_to_vaddr(vma, uprobe->offset);
> >  		err |= remove_breakpoint(uprobe, vma, vaddr);
> >  	}
> > -	mmap_read_unlock(mm);
> > +	mmap_write_unlock(mm);
> >  
> >  	return err;
> >  }
> > -- 
> > 2.49.0
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.