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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c8d88060dfbf..d256c695d7ff 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1483,7 +1483,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;
@@ -1500,7 +1500,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

On 04/21, Jiri Olsa wrote:
>
> @@ -1483,7 +1483,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);

So uprobe_write_opcode() is always called under down_write(), right?
Then this

	* Called with mm->mmap_lock held for read or write.

comment should be probably updated.

And perhaps the comment above mmap_write_lock() in register_for_each_vma()
should be updated too... or even removed.

Oleg.

On Sun, Apr 27, 2025 at 04:24:01PM +0200, Oleg Nesterov wrote:
> On 04/21, Jiri Olsa wrote:
> >
> > @@ -1483,7 +1483,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);
> 
> So uprobe_write_opcode() is always called under down_write(), right?
> Then this
> 
> 	* Called with mm->mmap_lock held for read or write.
> 
> comment should be probably updated.

yes

> 
> And perhaps the comment above mmap_write_lock() in register_for_each_vma()
> should be updated too... or even removed.

hum, not sure now how it's related to this change, but will stare at it bit more

thanks,
jirka

On 04/28, Jiri Olsa wrote:
>
> On Sun, Apr 27, 2025 at 04:24:01PM +0200, Oleg Nesterov wrote:
> >
> > And perhaps the comment above mmap_write_lock() in register_for_each_vma()
> > should be updated too... or even removed.
>
> hum, not sure now how it's related to this change, but will stare at it bit more

That comment tries to explain why register_for_each_vma() has to take
mm->mmap_lock for writing. Without the described race it could use
mmap_read_lock(). See 84455e6923c79 for the details.

Now that we have another (obvious) reason for mmap_write_lock(mm), this
comment looks confusing.

Oleg.

On Mon, Apr 21, 2025 at 2:45 PM 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.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/events/uprobes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Makes sense.

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index c8d88060dfbf..d256c695d7ff 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1483,7 +1483,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;
> @@ -1500,7 +1500,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
>