Re: [PATCH v2 02/12] uprobes: correct mmap_sem locking assumptions in uprobe_write_opcode()
On Wed, 3 Jul 2024 11:25:35 -0700 Andrii Nakryiko wrote: > On Wed, Jul 3, 2024 at 6:15 AM Masami Hiramatsu wrote: > > > > On Mon, 1 Jul 2024 15:39:25 -0700 > > Andrii Nakryiko wrote: > > > > > It seems like uprobe_write_opcode() doesn't require writer locked > > > mmap_sem, any lock (reader or writer) should be sufficient. This was > > > established in a discussion in [0] and looking through existing code > > > seems to confirm that there is no need for write-locked mmap_sem. > > > > > > Fix the comment to state this clearly. > > > > > > [0] > > > https://lore.kernel.org/linux-trace-kernel/20240625190748.gc14...@redhat.com/ > > > > > > Fixes: 29dedee0e693 ("uprobes: Add mem_cgroup_charge_anon() into > > > uprobe_write_opcode()") > > > > nit: why this has Fixes but [01/12] doesn't? > > > > Should I pick both to fixes branch? > > I'd keep both of them in probes/for-next, tbh. They are not literally > fixing anything, just cleaning up comments. I can drop the Fixes tag > from this one, if you'd like. Got it. Usually cleanup patch will not have Fixed tag, so if you don't mind, please drop it. Thank you, > > > > > Thank you, > > > > > Signed-off-by: Andrii Nakryiko > > > --- > > > kernel/events/uprobes.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > > index 081821fd529a..f87049c08ee9 100644 > > > --- a/kernel/events/uprobes.c > > > +++ b/kernel/events/uprobes.c > > > @@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, > > > struct mm_struct *mm, > > > * @vaddr: the virtual address to store the opcode. > > > * @opcode: opcode to be written at @vaddr. > > > * > > > - * Called with mm->mmap_lock held for write. > > > + * 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 mm_struct > > > *mm, > > > -- > > > 2.43.0 > > > > > > > > > -- > > Masami Hiramatsu (Google) -- Masami Hiramatsu (Google)
Re: [PATCH v2 02/12] uprobes: correct mmap_sem locking assumptions in uprobe_write_opcode()
On Wed, Jul 3, 2024 at 6:15 AM Masami Hiramatsu wrote: > > On Mon, 1 Jul 2024 15:39:25 -0700 > Andrii Nakryiko wrote: > > > It seems like uprobe_write_opcode() doesn't require writer locked > > mmap_sem, any lock (reader or writer) should be sufficient. This was > > established in a discussion in [0] and looking through existing code > > seems to confirm that there is no need for write-locked mmap_sem. > > > > Fix the comment to state this clearly. > > > > [0] > > https://lore.kernel.org/linux-trace-kernel/20240625190748.gc14...@redhat.com/ > > > > Fixes: 29dedee0e693 ("uprobes: Add mem_cgroup_charge_anon() into > > uprobe_write_opcode()") > > nit: why this has Fixes but [01/12] doesn't? > > Should I pick both to fixes branch? I'd keep both of them in probes/for-next, tbh. They are not literally fixing anything, just cleaning up comments. I can drop the Fixes tag from this one, if you'd like. > > Thank you, > > > Signed-off-by: Andrii Nakryiko > > --- > > kernel/events/uprobes.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > > index 081821fd529a..f87049c08ee9 100644 > > --- a/kernel/events/uprobes.c > > +++ b/kernel/events/uprobes.c > > @@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct > > mm_struct *mm, > > * @vaddr: the virtual address to store the opcode. > > * @opcode: opcode to be written at @vaddr. > > * > > - * Called with mm->mmap_lock held for write. > > + * 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 mm_struct *mm, > > -- > > 2.43.0 > > > > > -- > Masami Hiramatsu (Google)
Re: [PATCH v2 02/12] uprobes: correct mmap_sem locking assumptions in uprobe_write_opcode()
On Mon, 1 Jul 2024 15:39:25 -0700 Andrii Nakryiko wrote: > It seems like uprobe_write_opcode() doesn't require writer locked > mmap_sem, any lock (reader or writer) should be sufficient. This was > established in a discussion in [0] and looking through existing code > seems to confirm that there is no need for write-locked mmap_sem. > > Fix the comment to state this clearly. > > [0] > https://lore.kernel.org/linux-trace-kernel/20240625190748.gc14...@redhat.com/ > > Fixes: 29dedee0e693 ("uprobes: Add mem_cgroup_charge_anon() into > uprobe_write_opcode()") nit: why this has Fixes but [01/12] doesn't? Should I pick both to fixes branch? Thank you, > Signed-off-by: Andrii Nakryiko > --- > kernel/events/uprobes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 081821fd529a..f87049c08ee9 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct > mm_struct *mm, > * @vaddr: the virtual address to store the opcode. > * @opcode: opcode to be written at @vaddr. > * > - * Called with mm->mmap_lock held for write. > + * 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 mm_struct *mm, > -- > 2.43.0 > -- Masami Hiramatsu (Google)
Re: [PATCH v2 02/12] uprobes: correct mmap_sem locking assumptions in uprobe_write_opcode()
On 07/01, Andrii Nakryiko wrote: > > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct > mm_struct *mm, > * @vaddr: the virtual address to store the opcode. > * @opcode: opcode to be written at @vaddr. > * > - * Called with mm->mmap_lock held for write. > + * Called with mm->mmap_lock held for read or write. > * Return 0 (success) or a negative errno. Thanks, Acked-by: Oleg Nesterov I'll try to send the patch which explains the reasons for mmap_write_lock() in register_for_each_vma() later. Oleg.
[PATCH v2 02/12] uprobes: correct mmap_sem locking assumptions in uprobe_write_opcode()
It seems like uprobe_write_opcode() doesn't require writer locked mmap_sem, any lock (reader or writer) should be sufficient. This was established in a discussion in [0] and looking through existing code seems to confirm that there is no need for write-locked mmap_sem. Fix the comment to state this clearly. [0] https://lore.kernel.org/linux-trace-kernel/20240625190748.gc14...@redhat.com/ Fixes: 29dedee0e693 ("uprobes: Add mem_cgroup_charge_anon() into uprobe_write_opcode()") Signed-off-by: Andrii Nakryiko --- kernel/events/uprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 081821fd529a..f87049c08ee9 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm, * @vaddr: the virtual address to store the opcode. * @opcode: opcode to be written at @vaddr. * - * Called with mm->mmap_lock held for write. + * 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 mm_struct *mm, -- 2.43.0