uprobes: kill uprobes_srcu/uprobe_srcu_id
Subsystems (3)
Oleg Nesterov
maintainercover uprobes: kill uprobes_srcu/uprobe_srcu_id
Hello,
Basically the same changes we discussed before.
Changes:
- do not introduce is_swbp_at_addr_fast(), change is_swbp_at_addr() to check current->mm == mm as Peter suggested
- 2/7 is new
Oleg.
Oleg Nesterov
maintainerPATCH 1/7 uprobes: optimize is_swbp_at_addr() for current->mm
Change is_swbp_at_addr() to try to avoid the costly read_opcode() if mm == current->mm, __copy_from_user_inatomic() should succeed in the likely case.
Currently this optimization is not important, but we are going to add more is_swbp_at_addr(current->mm) callers.
--- kernel/events/uprobes.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +11 -1
@@ -333,10 +333,20 @@ static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)uprobe_opcode_t opcode;int result; +if (current->mm == mm) { +pagefault_disable (); + result =__copy_from_user_inatomic (&opcode, (void __user *)vaddr, +sizeof (opcode)); +pagefault_enable (); + +if (likely (result ==0 )) + goto out; + } + result =read_opcode (mm, vaddr, &opcode);if (result)return result; - +out:if (is_swbp_insn (&opcode))return 1 ;
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Optimize is_swbp_at_addr() for current-%3Emm
Commit-ID: a41d60e10a611f0addf2741e2d56c95e14bd5ee0 Gitweb: http://git.kernel.org/tip/a41d60e10a611f0addf2741e2d56c95e14bd5ee0 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:27:44 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:54 +0200
uprobes: Optimize is_swbp_at_addr() for current-%3Emm
Change is_swbp_at_addr() to try to avoid the costly read_opcode() if mm == current->mm, __copy_from_user_inatomic() should succeed in the likely case.
Currently this optimization is not important, but we are going to add more is_swbp_at_addr(current->mm) callers.
--- kernel/events/uprobes.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +11 -1
@@ -333,10 +333,20 @@ static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)uprobe_opcode_t opcode;int result; +if (current->mm == mm) { +pagefault_disable (); + result =__copy_from_user_inatomic (&opcode, (void __user *)vaddr, +sizeof (opcode)); +pagefault_enable (); + +if (likely (result ==0 )) + goto out; + } + result =read_opcode (mm, vaddr, &opcode);if (result)return result; - +out:if (is_swbp_insn (&opcode))return 1 ;
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Optimize is_swbp_at_addr() for current-> mm
In reply to Oleg NesterovCommit-ID: c00b275043adc14d668f36266b890f0c53d46640 Gitweb: http://git.kernel.org/tip/c00b275043adc14d668f36266b890f0c53d46640 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:27:44 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:13:59 +0200
uprobes: Optimize is_swbp_at_addr() for current->mm
Change is_swbp_at_addr() to try to avoid the costly read_opcode() if mm == current->mm, __copy_from_user_inatomic() should succeed in the likely case.
Currently this optimization is not important, but we are going to add more is_swbp_at_addr(current->mm) callers.
--- kernel/events/uprobes.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +11 -1
@@ -333,10 +333,20 @@ static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)uprobe_opcode_t opcode;int result; +if (current->mm == mm) { +pagefault_disable (); + result =__copy_from_user_inatomic (&opcode, (void __user *)vaddr, +sizeof (opcode)); +pagefault_enable (); + +if (likely (result ==0 )) + goto out; + } + result =read_opcode (mm, vaddr, &opcode);if (result)return result; - +out:if (is_swbp_insn (&opcode))return 1 ;
Oleg Nesterov
maintainerPATCH 2/7 uprobes: change read_opcode() to use FOLL_FORCE
In reply to Oleg Nesterovset_orig_insn()->read_opcode() should not fail if the probed task did mprotect() after uprobe_register(), change it to use FOLL_FORCE. Without FOLL_WRITE this doesn't have any "side" effect but allows to read the !VM_READ memory.
There is another reason for this change, we are going to use is_swbp_at_addr() from handle_swbp() which can race with another thread doing mprotect().
--- kernel/events/uprobes.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +1 -1
@@ -312,7 +312,7 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_void *vaddr_new;int ret; - ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,0 , &page,NULL ); + ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,1 , &page,NULL );if (ret <=0 )return ret;
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Change read_opcode() to use FOLL_FORCE
Commit-ID: 2c357a15491100e94de8e6105e6046ee3a71e05a Gitweb: http://git.kernel.org/tip/2c357a15491100e94de8e6105e6046ee3a71e05a Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:27:59 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:55 +0200
uprobes: Change read_opcode() to use FOLL_FORCE
set_orig_insn()->read_opcode() should not fail if the probed task did mprotect() after uprobe_register(), change it to use FOLL_FORCE. Without FOLL_WRITE this doesn't have any "side" effect but allows to read the !VM_READ memory.
There is another reason for this change, we are going to use is_swbp_at_addr() from handle_swbp() which can race with another thread doing mprotect().
--- kernel/events/uprobes.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +1 -1
@@ -312,7 +312,7 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_void *vaddr_new;int ret; - ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,0 , &page,NULL ); + ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,1 , &page,NULL );if (ret <=0 )return ret;
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Change read_opcode() to use FOLL_FORCE
In reply to Oleg NesterovCommit-ID: a3d7bb47937b3a40b9f0c75655e97b3bb6407cbe Gitweb: http://git.kernel.org/tip/a3d7bb47937b3a40b9f0c75655e97b3bb6407cbe Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:27:59 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:14:49 +0200
uprobes: Change read_opcode() to use FOLL_FORCE
set_orig_insn()->read_opcode() should not fail if the probed task did mprotect() after uprobe_register(), change it to use FOLL_FORCE. Without FOLL_WRITE this doesn't have any "side" effect but allows to read the !VM_READ memory.
There is another reason for this change, we are going to use is_swbp_at_addr() from handle_swbp() which can race with another thread doing mprotect().
--- kernel/events/uprobes.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/events/uprobes.c +1 -1
@@ -312,7 +312,7 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_void *vaddr_new;int ret; - ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,0 , &page,NULL ); + ret =get_user_pages (NULL , mm, vaddr,1 ,0 ,1 , &page,NULL );if (ret <=0 )return ret;
Oleg Nesterov
maintainerPATCH 3/7 uprobes: introduce find_active_uprobe() helper
In reply to Oleg NesterovNo functional changes. Move the "find uprobe" code from handle_swbp() to the new helper, find_active_uprobe().
Note: with or without this change, the find-active-uprobe logic is not exactly right. We can race with another thread which unmaps the memory with the valid uprobe before we take mm->mmap_sem. We can't find this uprobe simply because find_vma() fails. In this case we wrongly assume that this trap was not caused by uprobe and send the erroneous SIGTRAP. See the next changes.
--- kernel/events/uprobes.c | 47 ++++++++++++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 19 deletions(-)
kernel/events/uprobes.c +28 -19
@@ -1489,38 +1489,47 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -/* -* Run handler and ask thread to singlestep. -* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. -*/ -static void handle_swbp (struct pt_regs *regs) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) { +struct mm_struct *mm = current->mm; +struct uprobe *uprobe =NULL ;struct vm_area_struct *vma; -struct uprobe_task *utask; -struct uprobe *uprobe; -struct mm_struct *mm; -unsigned long bp_vaddr; - uprobe =NULL ; - bp_vaddr =uprobe_get_swbp_addr (regs); - mm = current->mm;down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr &&valid_vma (vma, false)) { -struct inode *inode; -loff_t offset; +if (vma && vma->vm_start <= bp_vaddr) { +if (valid_vma (vma, false)) { +struct inode *inode; +loff_t offset; - inode = vma->vm_file->f_mapping->host; - offset = bp_vaddr - vma->vm_start; - offset += (vma->vm_pgoff <<PAGE_SHIFT ); - uprobe =find_uprobe (inode, offset); + inode = vma->vm_file->f_mapping->host; + offset = bp_vaddr - vma->vm_start; + offset += (vma->vm_pgoff <<PAGE_SHIFT ); + uprobe =find_uprobe (inode, offset); + } }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem); +return uprobe; +} + +/* +* Run handler and ask thread to singlestep. +* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. +*/ +static void handle_swbp (struct pt_regs *regs) +{ +struct uprobe_task *utask; +struct uprobe *uprobe; +unsigned long bp_vaddr; + + bp_vaddr =uprobe_get_swbp_addr (regs); + uprobe =find_active_uprobe (bp_vaddr); +if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */ send_sig (SIGTRAP , current,0 );
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Introduce find_active_uprobe() helper
Commit-ID: 2a1bca6ff0ef8fd9b943ce2e6a8826606dc58e3f Gitweb: http://git.kernel.org/tip/2a1bca6ff0ef8fd9b943ce2e6a8826606dc58e3f Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:28:57 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:55 +0200
uprobes: Introduce find_active_uprobe() helper
No functional changes. Move the "find uprobe" code from handle_swbp() to the new helper, find_active_uprobe().
Note: with or without this change, the find-active-uprobe logic is not exactly right. We can race with another thread which unmaps the memory with the valid uprobe before we take mm->mmap_sem. We can't find this uprobe simply because find_vma() fails. In this case we wrongly assume that this trap was not caused by uprobe and send the erroneous SIGTRAP. See the next changes.
--- kernel/events/uprobes.c | 47 ++++++++++++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 19 deletions(-)
kernel/events/uprobes.c +28 -19
@@ -1489,38 +1489,47 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -/* -* Run handler and ask thread to singlestep. -* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. -*/ -static void handle_swbp (struct pt_regs *regs) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) { +struct mm_struct *mm = current->mm; +struct uprobe *uprobe =NULL ;struct vm_area_struct *vma; -struct uprobe_task *utask; -struct uprobe *uprobe; -struct mm_struct *mm; -unsigned long bp_vaddr; - uprobe =NULL ; - bp_vaddr =uprobe_get_swbp_addr (regs); - mm = current->mm;down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr &&valid_vma (vma, false)) { -struct inode *inode; -loff_t offset; +if (vma && vma->vm_start <= bp_vaddr) { +if (valid_vma (vma, false)) { +struct inode *inode; +loff_t offset; - inode = vma->vm_file->f_mapping->host; - offset = bp_vaddr - vma->vm_start; - offset += (vma->vm_pgoff <<PAGE_SHIFT ); - uprobe =find_uprobe (inode, offset); + inode = vma->vm_file->f_mapping->host; + offset = bp_vaddr - vma->vm_start; + offset += (vma->vm_pgoff <<PAGE_SHIFT ); + uprobe =find_uprobe (inode, offset); + } }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem); +return uprobe; +} + +/* +* Run handler and ask thread to singlestep. +* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. +*/ +static void handle_swbp (struct pt_regs *regs) +{ +struct uprobe_task *utask; +struct uprobe *uprobe; +unsigned long bp_vaddr; + + bp_vaddr =uprobe_get_swbp_addr (regs); + uprobe =find_active_uprobe (bp_vaddr); +if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */ send_sig (SIGTRAP , current,0 );
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Introduce find_active_uprobe() helper
In reply to Oleg NesterovCommit-ID: 3a9ea0520f38def4a3915b91f82455b749f07d88 Gitweb: http://git.kernel.org/tip/3a9ea0520f38def4a3915b91f82455b749f07d88 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:28:57 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:15:17 +0200
uprobes: Introduce find_active_uprobe() helper
No functional changes. Move the "find uprobe" code from handle_swbp() to the new helper, find_active_uprobe().
Note: with or without this change, the find-active-uprobe logic is not exactly right. We can race with another thread which unmaps the memory with the valid uprobe before we take mm->mmap_sem. We can't find this uprobe simply because find_vma() fails. In this case we wrongly assume that this trap was not caused by uprobe and send the erroneous SIGTRAP. See the next changes.
--- kernel/events/uprobes.c | 47 ++++++++++++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 19 deletions(-)
kernel/events/uprobes.c +28 -19
@@ -1489,38 +1489,47 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -/* -* Run handler and ask thread to singlestep. -* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. -*/ -static void handle_swbp (struct pt_regs *regs) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) { +struct mm_struct *mm = current->mm; +struct uprobe *uprobe =NULL ;struct vm_area_struct *vma; -struct uprobe_task *utask; -struct uprobe *uprobe; -struct mm_struct *mm; -unsigned long bp_vaddr; - uprobe =NULL ; - bp_vaddr =uprobe_get_swbp_addr (regs); - mm = current->mm;down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr &&valid_vma (vma, false)) { -struct inode *inode; -loff_t offset; +if (vma && vma->vm_start <= bp_vaddr) { +if (valid_vma (vma, false)) { +struct inode *inode; +loff_t offset; - inode = vma->vm_file->f_mapping->host; - offset = bp_vaddr - vma->vm_start; - offset += (vma->vm_pgoff <<PAGE_SHIFT ); - uprobe =find_uprobe (inode, offset); + inode = vma->vm_file->f_mapping->host; + offset = bp_vaddr - vma->vm_start; + offset += (vma->vm_pgoff <<PAGE_SHIFT ); + uprobe =find_uprobe (inode, offset); + } }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem); +return uprobe; +} + +/* +* Run handler and ask thread to singlestep. +* Ensure all non-fatal signals cannot interrupt thread while it singlesteps. +*/ +static void handle_swbp (struct pt_regs *regs) +{ +struct uprobe_task *utask; +struct uprobe *uprobe; +unsigned long bp_vaddr; + + bp_vaddr =uprobe_get_swbp_addr (regs); + uprobe =find_active_uprobe (bp_vaddr); +if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */ send_sig (SIGTRAP , current,0 );
Oleg Nesterov
maintainerPATCH 4/7 uprobes: teach find_active_uprobe() to provide the "is_swbp" info
In reply to Oleg NesterovA separate patch to simplify the review, and for the documentation.
The patch adds another "int *is_swbp" argument to find_active_uprobe(), so far its only caller doesn't use this info.
With this patch find_active_uprobe() additionally does:
- if find_vma() + ->vm_start check fails, *is_swbp = -EFAULT
- otherwise, if valid_vma() + find_uprobe() fails, it holds the result of is_swbp_at_addr(), can be negative too. The latter is only possible if we raced with another thread which did munmap/etc after we hit this bp.
IOW. If find_active_uprobe(&is_swbp) returns NULL, the caller can look at is_swbp to figure out whether the current insn is bp or not, or detect the race with another thread if it is negative.
Note: I think that performance-wise this change is fine. This adds is_swbp_at_addr(), but only if we raced with uprobe_unregister() or if we hit the "normal" int3 but this mm has uprobes as well. And even in this case the slow read_opcode() path is very unlikely, this insn recently triggered do_int3(), __copy_from_user_inatomic() shouldn't fail in the likely case.
--- kernel/events/uprobes.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +8 -3
@@ -1489,7 +1489,7 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr,int *is_swbp) {struct mm_struct *mm = current->mm;struct uprobe *uprobe =NULL ;
@@ -1497,7 +1497,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr)down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr) {if (valid_vma (vma, false)) {struct inode *inode;
@@ -1508,6 +1507,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr) offset += (vma->vm_pgoff <<PAGE_SHIFT ); uprobe =find_uprobe (inode, offset); } + +if (!uprobe) + *is_swbp =is_swbp_at_addr (mm, bp_vaddr); + }else { + *is_swbp = -EFAULT ; }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id);
@@ -1526,9 +1530,10 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; +int is_swbp; bp_vaddr =uprobe_get_swbp_addr (regs); - uprobe =find_active_uprobe (bp_vaddr); + uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Teach find_active_uprobe() to provide the %22is_swbp%22 info
Commit-ID: 183085e74a7faf3654c3a7c243b67b5cb76fa708 Gitweb: http://git.kernel.org/tip/183085e74a7faf3654c3a7c243b67b5cb76fa708 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:14 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:56 +0200
uprobes: Teach find_active_uprobe() to provide the %22is_swbp%22 info
A separate patch to simplify the review, and for the documentation.
The patch adds another "int *is_swbp" argument to find_active_uprobe(), so far its only caller doesn't use this info.
With this patch find_active_uprobe() additionally does:
- if find_vma() + ->vm_start check fails, *is_swbp = -EFAULT
- otherwise, if valid_vma() + find_uprobe() fails, it holds the result of is_swbp_at_addr(), can be negative too. The latter is only possible if we raced with another thread which did munmap/etc after we hit this bp.
IOW. If find_active_uprobe(&is_swbp) returns NULL, the caller can look at is_swbp to figure out whether the current insn is bp or not, or detect the race with another thread if it is negative.
Note: I think that performance-wise this change is fine. This adds is_swbp_at_addr(), but only if we raced with uprobe_unregister() or if we hit the "normal" int3 but this mm has uprobes as well. And even in this case the slow read_opcode() path is very unlikely, this insn recently triggered do_int3(), __copy_from_user_inatomic() shouldn't fail in the likely case.
--- kernel/events/uprobes.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +8 -3
@@ -1489,7 +1489,7 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr,int *is_swbp) {struct mm_struct *mm = current->mm;struct uprobe *uprobe =NULL ;
@@ -1497,7 +1497,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr)down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr) {if (valid_vma (vma, false)) {struct inode *inode;
@@ -1508,6 +1507,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr) offset += (vma->vm_pgoff <<PAGE_SHIFT ); uprobe =find_uprobe (inode, offset); } + +if (!uprobe) + *is_swbp =is_swbp_at_addr (mm, bp_vaddr); + }else { + *is_swbp = -EFAULT ; }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id);
@@ -1526,9 +1530,10 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; +int is_swbp; bp_vaddr =uprobe_get_swbp_addr (regs); - uprobe =find_active_uprobe (bp_vaddr); + uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Teach find_active_uprobe() to provide the "is_swbp" info
In reply to Oleg NesterovCommit-ID: d790d34653ab20c74034902f5f0889bba807949a Gitweb: http://git.kernel.org/tip/d790d34653ab20c74034902f5f0889bba807949a Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:14 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:15:24 +0200
uprobes: Teach find_active_uprobe() to provide the "is_swbp" info
A separate patch to simplify the review, and for the documentation.
The patch adds another "int *is_swbp" argument to find_active_uprobe(), so far its only caller doesn't use this info.
With this patch find_active_uprobe() additionally does:
- if find_vma() + ->vm_start check fails, *is_swbp = -EFAULT
- otherwise, if valid_vma() + find_uprobe() fails, it holds the result of is_swbp_at_addr(), can be negative too. The latter is only possible if we raced with another thread which did munmap/etc after we hit this bp.
IOW. If find_active_uprobe(&is_swbp) returns NULL, the caller can look at is_swbp to figure out whether the current insn is bp or not, or detect the race with another thread if it is negative.
Note: I think that performance-wise this change is fine. This adds is_swbp_at_addr(), but only if we raced with uprobe_unregister() or if we hit the "normal" int3 but this mm has uprobes as well. And even in this case the slow read_opcode() path is very unlikely, this insn recently triggered do_int3(), __copy_from_user_inatomic() shouldn't fail in the likely case.
--- kernel/events/uprobes.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +8 -3
@@ -1489,7 +1489,7 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)return false; } -static struct uprobe *find_active_uprobe (unsigned long bp_vaddr) +static struct uprobe *find_active_uprobe (unsigned long bp_vaddr,int *is_swbp) {struct mm_struct *mm = current->mm;struct uprobe *uprobe =NULL ;
@@ -1497,7 +1497,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr)down_read (&mm->mmap_sem); vma =find_vma (mm, bp_vaddr); -if (vma && vma->vm_start <= bp_vaddr) {if (valid_vma (vma, false)) {struct inode *inode;
@@ -1508,6 +1507,11 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr) offset += (vma->vm_pgoff <<PAGE_SHIFT ); uprobe =find_uprobe (inode, offset); } + +if (!uprobe) + *is_swbp =is_swbp_at_addr (mm, bp_vaddr); + }else { + *is_swbp = -EFAULT ; }srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id);
@@ -1526,9 +1530,10 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; +int is_swbp; bp_vaddr =uprobe_get_swbp_addr (regs); - uprobe =find_active_uprobe (bp_vaddr); + uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) {/* No matching uprobe; signal SIGTRAP. */
Oleg Nesterov
maintainerPATCH 5/7 uprobes: change register_for_each_vma() to take mm->mmap_sem for writing
In reply to Oleg NesterovChange register_for_each_vma() to take mm->mmap_sem for writing. This is a bit unfortunate but hopefully not too bad, this is the slow path anyway.
This is needed to ensure that find_active_uprobe() can not race with uprobe_register() which adds the new bp at the same bp_vaddr, after find_uprobe() fails and before is_swbp_at_addr_fast() checks the memory.
IOW, this is needed to ensure that if find_active_uprobe() returns NULL but is_swbp == true, we can safely assume that it was the "normal" int3 and we should send SIGTRAP.
There is another reason for this change. We are going to replace uprobes_state->count with MMF_ flags set by register/unregister and cleared by find_active_uprobe(), and set/clear shouldn't race with each other.
--- kernel/events/uprobes.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
kernel/events/uprobes.c +4 -4
@@ -853,12 +853,12 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) } mm = vi->mm; -down_read (&mm->mmap_sem); +down_write (&mm->mmap_sem); vma =find_vma (mm, (unsigned long )vi->vaddr);if (!vma || !valid_vma (vma, is_register)) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -867,7 +867,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) vaddr != vi->vaddr) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -877,7 +877,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)else remove_breakpoint (uprobe, mm, vi->vaddr); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);if (is_register) {if (ret && ret == -EEXIST )
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Change register_for_each_vma() to take mm-%3Emmap_sem for writing
Commit-ID: 8d65fee35e63f11f18b658df7c655f674c4bc660 Gitweb: http://git.kernel.org/tip/8d65fee35e63f11f18b658df7c655f674c4bc660 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:28 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:56 +0200
uprobes: Change register_for_each_vma() to take mm-%3Emmap_sem for writing
Change register_for_each_vma() to take mm->mmap_sem for writing. This is a bit unfortunate but hopefully not too bad, this is the slow path anyway.
This is needed to ensure that find_active_uprobe() can not race with uprobe_register() which adds the new bp at the same bp_vaddr, after find_uprobe() fails and before is_swbp_at_addr_fast() checks the memory.
IOW, this is needed to ensure that if find_active_uprobe() returns NULL but is_swbp == true, we can safely assume that it was the "normal" int3 and we should send SIGTRAP.
There is another reason for this change. We are going to replace uprobes_state->count with MMF_ flags set by register/unregister and cleared by find_active_uprobe(), and set/clear shouldn't race with each other.
--- kernel/events/uprobes.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
kernel/events/uprobes.c +4 -4
@@ -853,12 +853,12 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) } mm = vi->mm; -down_read (&mm->mmap_sem); +down_write (&mm->mmap_sem); vma =find_vma (mm, (unsigned long )vi->vaddr);if (!vma || !valid_vma (vma, is_register)) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -867,7 +867,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) vaddr != vi->vaddr) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -877,7 +877,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)else remove_breakpoint (uprobe, mm, vi->vaddr); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);if (is_register) {if (ret && ret == -EEXIST )
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Change register_for_each_vma() to take mm->mmap_sem for writing
In reply to Oleg NesterovCommit-ID: 77fc4af1b59d12ab3b1467adf0a5204806853123 Gitweb: http://git.kernel.org/tip/77fc4af1b59d12ab3b1467adf0a5204806853123 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:28 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:21:48 +0200
uprobes: Change register_for_each_vma() to take mm->mmap_sem for writing
Change register_for_each_vma() to take mm->mmap_sem for writing. This is a bit unfortunate but hopefully not too bad, this is the slow path anyway.
This is needed to ensure that find_active_uprobe() can not race with uprobe_register() which adds the new bp at the same bp_vaddr, after find_uprobe() fails and before is_swbp_at_addr_fast() checks the memory.
IOW, this is needed to ensure that if find_active_uprobe() returns NULL but is_swbp == true, we can safely assume that it was the "normal" int3 and we should send SIGTRAP.
There is another reason for this change. We are going to replace uprobes_state->count with MMF_ flags set by register/unregister and cleared by find_active_uprobe(), and set/clear shouldn't race with each other.
--- kernel/events/uprobes.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
kernel/events/uprobes.c +4 -4
@@ -853,12 +853,12 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) } mm = vi->mm; -down_read (&mm->mmap_sem); +down_write (&mm->mmap_sem); vma =find_vma (mm, (unsigned long )vi->vaddr);if (!vma || !valid_vma (vma, is_register)) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -867,7 +867,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register) vaddr != vi->vaddr) {list_del (&vi->probe_list);kfree (vi); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);continue ; }
@@ -877,7 +877,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)else remove_breakpoint (uprobe, mm, vi->vaddr); -up_read (&mm->mmap_sem); +up_write (&mm->mmap_sem);mmput (mm);if (is_register) {if (ret && ret == -EEXIST )
Oleg Nesterov
maintainerPATCH 6/7 uprobes: teach handle_swbp() to rely on "is_swbp" rather than uprobes_srcu
In reply to Oleg NesterovCurrently handle_swbp() assumes that it can't race with unregister, so it roughly does:
if (find_uprobe(vaddr)) process_uprobe(); else send_sig(SIGTRAP);
This relies on the not-really-working uprobes_srcu code we are going to remove, see the next patch.
With this patch we rely on the result of is_swbp_at_addr(bp_vaddr) if find_uprobe() fails.
If is_swbp == 1, then we hit the normal int3, we should send SIGTRAP.
If is_swbp == 0, we raced with uprobe_unregister(), we simply restart this insn again.
The "difficult" case is is_swbp == -EFAULT, when we can't read this memory. In this case I think we should restart too, and this is more correct compared to the current code which sends SIGTRAP.
Ignoring ENOMEM/etc from get_user_pages(), this can only happen if another thread unmaps this memory before find_active_uprobe() takes mmap_sem. It would be better to pretend it was unmapped before this insn was executed, restart, and get SIGSEGV.
--- kernel/events/uprobes.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +15 -3
@@ -1530,14 +1530,26 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; -int is_swbp; +int uninitialized_var (is_swbp ); bp_vaddr =uprobe_get_swbp_addr (regs); uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) { -/* No matching uprobe; signal SIGTRAP. */ -send_sig (SIGTRAP , current,0 ); +if (is_swbp >0 ) { +/* No matching uprobe; signal SIGTRAP. */ +send_sig (SIGTRAP , current,0 ); + }else { +/* +* Either we raced with uprobe_unregister() or we can't +* access this memory. The latter is only possible if +* another thread plays with our ->mm. In both cases +* we can simply restart. If this vma was unmapped we +* can pretend this insn was not executed yet and get +* the (correct) SIGSEGV after restart. +*/ +instruction_pointer_set (regs, bp_vaddr); + }return ; }
Signature · 1 line
1.5.5.1
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Teach handle_swbp() to rely on %22is_swbp%22 rather than uprobes_srcu
Commit-ID: 27e471e0846354376d180c2d2911ec9b94f1e28f Gitweb: http://git.kernel.org/tip/27e471e0846354376d180c2d2911ec9b94f1e28f Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:47 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:57 +0200
uprobes: Teach handle_swbp() to rely on %22is_swbp%22 rather than uprobes_srcu
Currently handle_swbp() assumes that it can't race with unregister, so it roughly does:
if (find_uprobe(vaddr)) process_uprobe(); else send_sig(SIGTRAP);
This relies on the not-really-working uprobes_srcu code we are going to remove, see the next patch.
With this patch we rely on the result ofis_swbp_at_addr(bp_vaddr) if find_uprobe() fails.
If is_swbp == 1, then we hit the normal int3, we should send SIGTRAP.
If is_swbp == 0, we raced with uprobe_unregister(), we simply restart this insn again.
The "difficult" case is is_swbp == -EFAULT, when we can't read this memory. In this case I think we should restart too, and this is more correct compared to the current code which sends SIGTRAP.
Ignoring ENOMEM/etc from get_user_pages(), this can only happen if another thread unmaps this memory before find_active_uprobe() takes mmap_sem. It would be better to pretend it was unmapped before this insn was executed, restart, and get SIGSEGV.
--- kernel/events/uprobes.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +15 -3
@@ -1530,14 +1530,26 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; -int is_swbp; +int uninitialized_var (is_swbp ); bp_vaddr =uprobe_get_swbp_addr (regs); uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) { -/* No matching uprobe; signal SIGTRAP. */ -send_sig (SIGTRAP , current,0 ); +if (is_swbp >0 ) { +/* No matching uprobe; signal SIGTRAP. */ +send_sig (SIGTRAP , current,0 ); + }else { +/* +* Either we raced with uprobe_unregister() or we can't +* access this memory. The latter is only possible if +* another thread plays with our ->mm. In both cases +* we can simply restart. If this vma was unmapped we +* can pretend this insn was not executed yet and get +* the (correct) SIGSEGV after restart. +*/ +instruction_pointer_set (regs, bp_vaddr); + }return ; }
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Teach handle_swbp() to rely on "is_swbp" rather than uprobes_srcu
In reply to Oleg NesterovCommit-ID: 56bb4cf6475d702d2fb00fc641aa6441097c0330 Gitweb: http://git.kernel.org/tip/56bb4cf6475d702d2fb00fc641aa6441097c0330 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:29:47 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:22:03 +0200
uprobes: Teach handle_swbp() to rely on "is_swbp" rather than uprobes_srcu
Currently handle_swbp() assumes that it can't race with unregister, so it roughly does:
if (find_uprobe(vaddr)) process_uprobe(); else send_sig(SIGTRAP);
This relies on the not-really-working uprobes_srcu code we are going to remove, see the next patch.
With this patch we rely on the result ofis_swbp_at_addr(bp_vaddr) if find_uprobe() fails.
If is_swbp == 1, then we hit the normal int3, we should send SIGTRAP.
If is_swbp == 0, we raced with uprobe_unregister(), we simply restart this insn again.
The "difficult" case is is_swbp == -EFAULT, when we can't read this memory. In this case I think we should restart too, and this is more correct compared to the current code which sends SIGTRAP.
Ignoring ENOMEM/etc from get_user_pages(), this can only happen if another thread unmaps this memory before find_active_uprobe() takes mmap_sem. It would be better to pretend it was unmapped before this insn was executed, restart, and get SIGSEGV.
--- kernel/events/uprobes.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
kernel/events/uprobes.c +15 -3
@@ -1530,14 +1530,26 @@ static void handle_swbp(struct pt_regs *regs)struct uprobe_task *utask;struct uprobe *uprobe;unsigned long bp_vaddr; -int is_swbp; +int uninitialized_var (is_swbp ); bp_vaddr =uprobe_get_swbp_addr (regs); uprobe =find_active_uprobe (bp_vaddr, &is_swbp);if (!uprobe) { -/* No matching uprobe; signal SIGTRAP. */ -send_sig (SIGTRAP , current,0 ); +if (is_swbp >0 ) { +/* No matching uprobe; signal SIGTRAP. */ +send_sig (SIGTRAP , current,0 ); + }else { +/* +* Either we raced with uprobe_unregister() or we can't +* access this memory. The latter is only possible if +* another thread plays with our ->mm. In both cases +* we can simply restart. If this vma was unmapped we +* can pretend this insn was not executed yet and get +* the (correct) SIGSEGV after restart. +*/ +instruction_pointer_set (regs, bp_vaddr); + }return ; }
Oleg Nesterov
maintainerPATCH 7/7 uprobes: kill uprobes_srcu/uprobe_srcu_id
In reply to Oleg NesterovKill the no longer needed uprobes_srcu/uprobe_srcu_id code.
It doesn't really work anyway. synchronize_srcu() can only synchronize with the code "inside" the srcu_read_lock/srcu_read_unlock section, while uprobe_pre_sstep_notifier() does srcu_read_lock() _after_ we already hit the breakpoint.
I guess this probably works "in practice". synchronize_srcu() is slow and it implies synchronize_sched(), and the probed task enters the non- preemptible section at the start of exception handler. Still this is not right at least in theory, and task->uprobe_srcu_id blows task_struct.
--- include/linux/sched.h | 1 - kernel/events/uprobes.c | 22 +++------------------- 2 files changed, 3 insertions(+), 20 deletions(-)
include/linux/sched.h +0 -1
@@ -1574,7 +1574,6 @@ struct task_struct {#endif #ifdef CONFIG_UPROBES struct uprobe_task *utask; -int uprobe_srcu_id;#endif };
kernel/events/uprobes.c +3 -19
@@ -38,7 +38,6 @@#define UINSNS_PER_PAGE (PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)#define MAX_UPROBE_XOL_SLOTS UINSNS_PER_PAGE -static struct srcu_struct uprobes_srcu;static struct rb_root uprobes_tree =RB_ROOT ;static DEFINE_SPINLOCK (uprobes_treelock);/* serialize rbtree access */
@@ -738,20 +737,14 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr) }/* -* There could be threads that have hit the breakpoint and are entering the -* notifier code and trying to acquire the uprobes_treelock. The thread -* calling delete_uprobe() that is removing the uprobe from the rb_tree can -* race with these threads and might acquire the uprobes_treelock compared -* to some of the breakpoint hit threads. In such a case, the breakpoint -* hit threads will not find the uprobe. The current unregistering thread -* waits till all other threads have hit a breakpoint, to acquire the -* uprobes_treelock before the uprobe is removed from the rbtree. +* There could be threads that have already hit the breakpoint. They +* will recheck the current insn and restart if find_uprobe() fails. +* See find_active_uprobe(). */ static void delete_uprobe (struct uprobe *uprobe) {unsigned long flags; -synchronize_srcu (&uprobes_srcu);spin_lock_irqsave (&uprobes_treelock, flags);rb_erase (&uprobe->rb_node, &uprobes_tree);spin_unlock_irqrestore (&uprobes_treelock, flags);
@@ -1388,9 +1381,6 @@ void uprobe_free_utask(struct task_struct *t) {struct uprobe_task *utask = t->utask; -if (t->uprobe_srcu_id !=-1 ) -srcu_read_unlock_raw (&uprobes_srcu, t->uprobe_srcu_id); -if (!utask)return ;
@@ -1408,7 +1398,6 @@ void uprobe_free_utask(struct task_struct *t)void uprobe_copy_process (struct task_struct *t) { t->utask =NULL ; - t->uprobe_srcu_id =-1 ; }/*
@@ -1513,9 +1502,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) }else { *is_swbp = -EFAULT ; } - -srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); - current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem);return uprobe;
@@ -1656,7 +1642,6 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs) utask->state =UTASK_BP_HIT ;set_thread_flag (TIF_UPROBE ); - current->uprobe_srcu_id =srcu_read_lock_raw (&uprobes_srcu);return 1 ; }
@@ -1691,7 +1676,6 @@ static int __init init_uprobes(void)mutex_init (&uprobes_mutex[i]);mutex_init (&uprobes_mmap_mutex[i]); } -init_srcu_struct (&uprobes_srcu);return register_die_notifier (&uprobe_exception_nb); }
Signature · 1 line
1.5.5.1
Peter Zijlstra
maintainerQuoted 11 lines · Oleg Nesterov
Kill the no longer needed uprobes_srcu/uprobe_srcu_id code.
It doesn't really work anyway.
synchronize_srcu()can only synchronize with the code "inside" the srcu_read_lock/srcu_read_unlock section, whileuprobe_pre_sstep_notifier()doessrcu_read_lock()_after_ we already hit the breakpoint.I guess this probably works "in practice".
synchronize_srcu()is slow and it impliessynchronize_sched(), and the probed task enters the non- preemptible section at the start of exception handler. Still this is not right at least in theory, andtask->uprobe_srcu_idblowstask_struct.
This kills the only user of srcu_read_{,un}lock_raw(), so I guess we could also make:
9ceae0e2
101db7b4
0c53dd8b
go away..
Quoted 13 lines · Peter Zijlstra
Quoted 11 lines · Oleg Nesterov
Kill the no longer needed uprobes_srcu/uprobe_srcu_id code.
It doesn't really work anyway.
synchronize_srcu()can only synchronize with the code "inside" the srcu_read_lock/srcu_read_unlock section, whileuprobe_pre_sstep_notifier()doessrcu_read_lock()_after_ we already hit the breakpoint.I guess this probably works "in practice".
synchronize_srcu()is slow and it impliessynchronize_sched(), and the probed task enters the non- preemptible section at the start of exception handler. Still this is not right at least in theory, andtask->uprobe_srcu_idblowstask_struct.
Quoted 8 lines
This kills the only user of srcu_read_{,un}lock_raw(), so I guess we could also make:
9ceae0e2
101db7b4
0c53dd8bgo away..
If it is still unused in a few months, I will get rid of them.
Thanx, Paul
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Kill uprobes_srcu/uprobe_srcu_id
In reply to Oleg NesterovCommit-ID: 71adc7a026a8c18db0f85d50d28c5f61a0b8d489 Gitweb: http://git.kernel.org/tip/71adc7a026a8c18db0f85d50d28c5f61a0b8d489 Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:30:08 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 11:30:57 +0200
uprobes: Kill uprobes_srcu/uprobe_srcu_id
Kill the no longer needed uprobes_srcu/uprobe_srcu_id code.
It doesn't really work anyway. synchronize_srcu() can only synchronize with the code "inside" the srcu_read_lock/srcu_read_unlock section, while uprobe_pre_sstep_notifier() does srcu_read_lock() _after_ we already hit the breakpoint.
I guess this probably works "in practice". synchronize_srcu() is slow and it implies synchronize_sched(), and the probed task enters the non- preemptible section at the start of exception handler. Still this is not right at least in theory, and task->uprobe_srcu_id blows task_struct.
--- include/linux/sched.h | 1 - kernel/events/uprobes.c | 22 +++------------------- 2 files changed, 3 insertions(+), 20 deletions(-)
include/linux/sched.h +0 -1
@@ -1569,7 +1569,6 @@ struct task_struct {#endif #ifdef CONFIG_UPROBES struct uprobe_task *utask; -int uprobe_srcu_id;#endif };
kernel/events/uprobes.c +3 -19
@@ -38,7 +38,6 @@#define UINSNS_PER_PAGE (PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)#define MAX_UPROBE_XOL_SLOTS UINSNS_PER_PAGE -static struct srcu_struct uprobes_srcu;static struct rb_root uprobes_tree =RB_ROOT ;static DEFINE_SPINLOCK (uprobes_treelock);/* serialize rbtree access */
@@ -738,20 +737,14 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr) }/* -* There could be threads that have hit the breakpoint and are entering the -* notifier code and trying to acquire the uprobes_treelock. The thread -* calling delete_uprobe() that is removing the uprobe from the rb_tree can -* race with these threads and might acquire the uprobes_treelock compared -* to some of the breakpoint hit threads. In such a case, the breakpoint -* hit threads will not find the uprobe. The current unregistering thread -* waits till all other threads have hit a breakpoint, to acquire the -* uprobes_treelock before the uprobe is removed from the rbtree. +* There could be threads that have already hit the breakpoint. They +* will recheck the current insn and restart if find_uprobe() fails. +* See find_active_uprobe(). */ static void delete_uprobe (struct uprobe *uprobe) {unsigned long flags; -synchronize_srcu (&uprobes_srcu);spin_lock_irqsave (&uprobes_treelock, flags);rb_erase (&uprobe->rb_node, &uprobes_tree);spin_unlock_irqrestore (&uprobes_treelock, flags);
@@ -1388,9 +1381,6 @@ void uprobe_free_utask(struct task_struct *t) {struct uprobe_task *utask = t->utask; -if (t->uprobe_srcu_id !=-1 ) -srcu_read_unlock_raw (&uprobes_srcu, t->uprobe_srcu_id); -if (!utask)return ;
@@ -1408,7 +1398,6 @@ void uprobe_free_utask(struct task_struct *t)void uprobe_copy_process (struct task_struct *t) { t->utask =NULL ; - t->uprobe_srcu_id =-1 ; }/*
@@ -1513,9 +1502,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) }else { *is_swbp = -EFAULT ; } - -srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); - current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem);return uprobe;
@@ -1656,7 +1642,6 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs) utask->state =UTASK_BP_HIT ;set_thread_flag (TIF_UPROBE ); - current->uprobe_srcu_id =srcu_read_lock_raw (&uprobes_srcu);return 1 ; }
@@ -1691,7 +1676,6 @@ static int __init init_uprobes(void)mutex_init (&uprobes_mutex[i]);mutex_init (&uprobes_mmap_mutex[i]); } -init_srcu_struct (&uprobes_srcu);return register_die_notifier (&uprobe_exception_nb); }
tip-bot for Oleg Nesterov
maintainer[tip:perf/core] uprobes: Kill uprobes_srcu/uprobe_srcu_id
In reply to Oleg NesterovCommit-ID: 778b032d96909690c19d84f8d17c13be65ed6f8e Gitweb: http://git.kernel.org/tip/778b032d96909690c19d84f8d17c13be65ed6f8e Author: Oleg Nesterov <[email protected]> AuthorDate: Tue, 29 May 2012 21:30:08 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 6 Jun 2012 17:22:22 +0200
uprobes: Kill uprobes_srcu/uprobe_srcu_id
Kill the no longer needed uprobes_srcu/uprobe_srcu_id code.
It doesn't really work anyway. synchronize_srcu() can only synchronize with the code "inside" the srcu_read_lock/srcu_read_unlock section, while uprobe_pre_sstep_notifier() does srcu_read_lock() _after_ we already hit the breakpoint.
I guess this probably works "in practice". synchronize_srcu() is slow and it implies synchronize_sched(), and the probed task enters the non- preemptible section at the start of exception handler. Still this is not right at least in theory, and task->uprobe_srcu_id blows task_struct.
--- include/linux/sched.h | 1 - kernel/events/uprobes.c | 22 +++------------------- 2 files changed, 3 insertions(+), 20 deletions(-)
include/linux/sched.h +0 -1
@@ -1569,7 +1569,6 @@ struct task_struct {#endif #ifdef CONFIG_UPROBES struct uprobe_task *utask; -int uprobe_srcu_id;#endif };
kernel/events/uprobes.c +3 -19
@@ -38,7 +38,6 @@#define UINSNS_PER_PAGE (PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)#define MAX_UPROBE_XOL_SLOTS UINSNS_PER_PAGE -static struct srcu_struct uprobes_srcu;static struct rb_root uprobes_tree =RB_ROOT ;static DEFINE_SPINLOCK (uprobes_treelock);/* serialize rbtree access */
@@ -738,20 +737,14 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr) }/* -* There could be threads that have hit the breakpoint and are entering the -* notifier code and trying to acquire the uprobes_treelock. The thread -* calling delete_uprobe() that is removing the uprobe from the rb_tree can -* race with these threads and might acquire the uprobes_treelock compared -* to some of the breakpoint hit threads. In such a case, the breakpoint -* hit threads will not find the uprobe. The current unregistering thread -* waits till all other threads have hit a breakpoint, to acquire the -* uprobes_treelock before the uprobe is removed from the rbtree. +* There could be threads that have already hit the breakpoint. They +* will recheck the current insn and restart if find_uprobe() fails. +* See find_active_uprobe(). */ static void delete_uprobe (struct uprobe *uprobe) {unsigned long flags; -synchronize_srcu (&uprobes_srcu);spin_lock_irqsave (&uprobes_treelock, flags);rb_erase (&uprobe->rb_node, &uprobes_tree);spin_unlock_irqrestore (&uprobes_treelock, flags);
@@ -1388,9 +1381,6 @@ void uprobe_free_utask(struct task_struct *t) {struct uprobe_task *utask = t->utask; -if (t->uprobe_srcu_id !=-1 ) -srcu_read_unlock_raw (&uprobes_srcu, t->uprobe_srcu_id); -if (!utask)return ;
@@ -1408,7 +1398,6 @@ void uprobe_free_utask(struct task_struct *t)void uprobe_copy_process (struct task_struct *t) { t->utask =NULL ; - t->uprobe_srcu_id =-1 ; }/*
@@ -1513,9 +1502,6 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) }else { *is_swbp = -EFAULT ; } - -srcu_read_unlock_raw (&uprobes_srcu, current->uprobe_srcu_id); - current->uprobe_srcu_id =-1 ;up_read (&mm->mmap_sem);return uprobe;
@@ -1656,7 +1642,6 @@ int uprobe_pre_sstep_notifier(struct pt_regs *regs) utask->state =UTASK_BP_HIT ;set_thread_flag (TIF_UPROBE ); - current->uprobe_srcu_id =srcu_read_lock_raw (&uprobes_srcu);return 1 ; }
@@ -1691,7 +1676,6 @@ static int __init init_uprobes(void)mutex_init (&uprobes_mutex[i]);mutex_init (&uprobes_mmap_mutex[i]); } -init_srcu_struct (&uprobes_srcu);return register_die_notifier (&uprobe_exception_nb); }
Quoted 10 lines
Basically the same changes we discussed before.
Changes:
- do not introduce
is_swbp_at_addr_fast(), changeis_swbp_at_addr()to checkcurrent->mm== mm as Peter suggested- 2/7 is new
I have reviewed and tested the patches and I am fine with all the changes.
Thanks Oleg.
Signature · 2 lines
Thanks and Regards Srikar