Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
* Oleg Nesterov [2012-07-28 18:31:57]: > https://bugzilla.redhat.com/show_bug.cgi?id=843640 > > If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path > does unmap_region() but does not remove the soon-to-be-freed vma > from rb tree (actually there are more problems). > > Perhaps we could do do_munmap() + return in this case, but in fact > it is simply wrong to abort if uprobe_mmap() fails. Until at least > we move the !UPROBE_COPY_INSN code from install_breakpoint() to > uprobe_register(). > > For example, uprobe_mmap()->install_breakpoint() can fail if the > probed insn is not supported (remember, uprobe_register() succeeds > if nobody mmaps inode/offset), mmap() should not fail in this case. > > dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() > can race with uprobe_register() and fail for no reason if it wins > the race and does install_breakpoint() first. > > Change mmap_region() and dup_mmap() to ignore the error code from > uprobe_mmap(). > > Reported-by: William Cohen > Signed-off-by: Oleg Nesterov > Cc: # v3.5 After discussions with Oleg, its very clear that this apt fix for now. Acked-by: Srikar Dronamraju > --- > kernel/fork.c |4 ++-- > mm/mmap.c |5 ++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index ab5211b..54bb88a 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct > mm_struct *oldmm) > if (retval) > goto out; > > - if (file && uprobe_mmap(tmp)) > - goto out; > + if (file) > + uprobe_mmap(tmp); > } > /* a new mm has just been created */ > arch_dup_mmap(oldmm, mm); > diff --git a/mm/mmap.c b/mm/mmap.c > index 4fe2697..f25fd3f 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1355,9 +1355,8 @@ out: > } else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK)) > make_pages_present(addr, addr + len); > > - if (file && uprobe_mmap(vma)) > - /* matching probes but cannot insert */ > - goto unmap_and_free_vma; > + if (file) > + uprobe_mmap(vma); > > return addr; > > -- > 1.5.5.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
* Oleg Nesterov o...@redhat.com [2012-07-28 18:31:57]: https://bugzilla.redhat.com/show_bug.cgi?id=843640 If mmap_region()-uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register(). For example, uprobe_mmap()-install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case. dup_mmap()-uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first. Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap(). Reported-by: William Cohen wco...@redhat.com Signed-off-by: Oleg Nesterov o...@redhat.com Cc: sta...@vger.kernel.org # v3.5 After discussions with Oleg, its very clear that this apt fix for now. Acked-by: Srikar Dronamraju sri...@linux.vnet.ibm.com --- kernel/fork.c |4 ++-- mm/mmap.c |5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..54bb88a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); } /* a new mm has just been created */ arch_dup_mmap(oldmm, mm); diff --git a/mm/mmap.c b/mm/mmap.c index 4fe2697..f25fd3f 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1355,9 +1355,8 @@ out: } else if ((flags MAP_POPULATE) !(flags MAP_NONBLOCK)) make_pages_present(addr, addr + len); - if (file uprobe_mmap(vma)) - /* matching probes but cannot insert */ - goto unmap_and_free_vma; + if (file) + uprobe_mmap(vma); return addr; -- 1.5.5.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
On 07/31, Oleg Nesterov wrote: > > OK, so what you suggest for now? > > Please note that it is very trivial to crash the kernel. Just > do something like > > echo "p /bin/true:OFFSET_OF_SYSCALL_INSN" > > /sys/kernel/debug/tracing/uprobe_events > /bin/true Forgot to mention... And even it it didn't crash, mmap() (and thus exec) should not fail. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
On 07/31, Srikar Dronamraju wrote: > > > --- a/kernel/fork.c > > +++ b/kernel/fork.c > > @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct > > mm_struct *oldmm) > > if (retval) > > goto out; > > > > - if (file && uprobe_mmap(tmp)) > > - goto out; > > + if (file) > > + uprobe_mmap(tmp); > > I am not comfortable with this fix. OK, so what you suggest for now? Please note that it is very trivial to crash the kernel. Just do something like echo "p /bin/true:OFFSET_OF_SYSCALL_INSN" > /sys/kernel/debug/tracing/uprobe_events /bin/true (or any other unsupported insn) Yes sure, I agree that in the long term this change should be reconsidered. > I think the long term solution is as you mentioned, move the > instruction analysis to register. Exactly. And we already discusssed this, we have a lot of other reasons to do this. > Lets say there were 10 probes that were to be installed in that vma. > we were able to install five probes and the 6th one happened to fail > because of invalid instruction; we dont continue with the registering > probes for the remaining 4 probes. Yes. And I already have the patch. I didn't send it because, unlike this fix, it depends on other changes (already in -tip). Until we move analysis to register, until we teach the callers of uprobe_mmap() to bailout (and please note that vma_adjust() ignores the result too), uprobe_mmap() should not give up if install fails, it should continue. > The intention behind failing mmap()/fork() if uprobe_mmap failed, > was to make sure that we always report the correct number of events. Sure, I understand and agree. But what we can do right now? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
> > If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path > does unmap_region() but does not remove the soon-to-be-freed vma > from rb tree (actually there are more problems). > > Perhaps we could do do_munmap() + return in this case, but in fact > it is simply wrong to abort if uprobe_mmap() fails. Until at least > we move the !UPROBE_COPY_INSN code from install_breakpoint() to > uprobe_register(). > > For example, uprobe_mmap()->install_breakpoint() can fail if the > probed insn is not supported (remember, uprobe_register() succeeds > if nobody mmaps inode/offset), mmap() should not fail in this case. > > dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() > can race with uprobe_register() and fail for no reason if it wins > the race and does install_breakpoint() first. > > Change mmap_region() and dup_mmap() to ignore the error code from > uprobe_mmap(). > > Reported-by: William Cohen > Signed-off-by: Oleg Nesterov > Cc: # v3.5 > --- > kernel/fork.c |4 ++-- > mm/mmap.c |5 ++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index ab5211b..54bb88a 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct > mm_struct *oldmm) > if (retval) > goto out; > > - if (file && uprobe_mmap(tmp)) > - goto out; > + if (file) > + uprobe_mmap(tmp); I am not comfortable with this fix. Lets say there were 10 probes that were to be installed in that vma. we were able to install five probes and the 6th one happened to fail because of invalid instruction; we dont continue with the registering probes for the remaining 4 probes. Your fix allows probe hits for 5 registered probes that can lead to misleading analysis. For example if one of the first five probes was a malloc and the probe at free() was one of the last probes which wasnt registered, then a person doing an analysis based on probes can say there was a memory leak. The intention behind failing mmap()/fork() if uprobe_mmap failed, was to make sure that we always report the correct number of events. Infact this was something that Peter advocated very strongly http://article.gmane.org/gmane.linux.kernel.mm/59956 I think the long term solution is as you mentioned, move the instruction analysis to register. Till then should we just ignore invalid instruction probes similar to existing probes. i.e we return -ESRCH or some such which is ignored in uprobe_mmap(), but is taken care of in the uprobe_register path. The above may not be an elegant solution but.. -- thanks and regards Srikar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
If mmap_region()-uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register(). For example, uprobe_mmap()-install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case. dup_mmap()-uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first. Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap(). Reported-by: William Cohen wco...@redhat.com Signed-off-by: Oleg Nesterov o...@redhat.com Cc: sta...@vger.kernel.org # v3.5 --- kernel/fork.c |4 ++-- mm/mmap.c |5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..54bb88a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); I am not comfortable with this fix. Lets say there were 10 probes that were to be installed in that vma. we were able to install five probes and the 6th one happened to fail because of invalid instruction; we dont continue with the registering probes for the remaining 4 probes. Your fix allows probe hits for 5 registered probes that can lead to misleading analysis. For example if one of the first five probes was a malloc and the probe at free() was one of the last probes which wasnt registered, then a person doing an analysis based on probes can say there was a memory leak. The intention behind failing mmap()/fork() if uprobe_mmap failed, was to make sure that we always report the correct number of events. Infact this was something that Peter advocated very strongly http://article.gmane.org/gmane.linux.kernel.mm/59956 I think the long term solution is as you mentioned, move the instruction analysis to register. Till then should we just ignore invalid instruction probes similar to existing probes. i.e we return -ESRCH or some such which is ignored in uprobe_mmap(), but is taken care of in the uprobe_register path. The above may not be an elegant solution but.. -- thanks and regards Srikar -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
On 07/31, Srikar Dronamraju wrote: --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); I am not comfortable with this fix. OK, so what you suggest for now? Please note that it is very trivial to crash the kernel. Just do something like echo p /bin/true:OFFSET_OF_SYSCALL_INSN /sys/kernel/debug/tracing/uprobe_events /bin/true (or any other unsupported insn) Yes sure, I agree that in the long term this change should be reconsidered. I think the long term solution is as you mentioned, move the instruction analysis to register. Exactly. And we already discusssed this, we have a lot of other reasons to do this. Lets say there were 10 probes that were to be installed in that vma. we were able to install five probes and the 6th one happened to fail because of invalid instruction; we dont continue with the registering probes for the remaining 4 probes. Yes. And I already have the patch. I didn't send it because, unlike this fix, it depends on other changes (already in -tip). Until we move analysis to register, until we teach the callers of uprobe_mmap() to bailout (and please note that vma_adjust() ignores the result too), uprobe_mmap() should not give up if install fails, it should continue. The intention behind failing mmap()/fork() if uprobe_mmap failed, was to make sure that we always report the correct number of events. Sure, I understand and agree. But what we can do right now? Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
On 07/31, Oleg Nesterov wrote: OK, so what you suggest for now? Please note that it is very trivial to crash the kernel. Just do something like echo p /bin/true:OFFSET_OF_SYSCALL_INSN /sys/kernel/debug/tracing/uprobe_events /bin/true Forgot to mention... And even it it didn't crash, mmap() (and thus exec) should not fail. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
On 07/28/2012 12:31 PM, Oleg Nesterov wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=843640 Hi Oleg, I checked the following patch and it does fix the problem on the 3.5.0+ kernel. -Will > > If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path > does unmap_region() but does not remove the soon-to-be-freed vma > from rb tree (actually there are more problems). > > Perhaps we could do do_munmap() + return in this case, but in fact > it is simply wrong to abort if uprobe_mmap() fails. Until at least > we move the !UPROBE_COPY_INSN code from install_breakpoint() to > uprobe_register(). > > For example, uprobe_mmap()->install_breakpoint() can fail if the > probed insn is not supported (remember, uprobe_register() succeeds > if nobody mmaps inode/offset), mmap() should not fail in this case. > > dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() > can race with uprobe_register() and fail for no reason if it wins > the race and does install_breakpoint() first. > > Change mmap_region() and dup_mmap() to ignore the error code from > uprobe_mmap(). > > Reported-by: William Cohen > Signed-off-by: Oleg Nesterov > Cc: # v3.5 > --- > kernel/fork.c |4 ++-- > mm/mmap.c |5 ++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index ab5211b..54bb88a 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct > mm_struct *oldmm) > if (retval) > goto out; > > - if (file && uprobe_mmap(tmp)) > - goto out; > + if (file) > + uprobe_mmap(tmp); > } > /* a new mm has just been created */ > arch_dup_mmap(oldmm, mm); > diff --git a/mm/mmap.c b/mm/mmap.c > index 4fe2697..f25fd3f 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1355,9 +1355,8 @@ out: > } else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK)) > make_pages_present(addr, addr + len); > > - if (file && uprobe_mmap(vma)) > - /* matching probes but cannot insert */ > - goto unmap_and_free_vma; > + if (file) > + uprobe_mmap(vma); > > return addr; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
On 07/28/2012 12:31 PM, Oleg Nesterov wrote: https://bugzilla.redhat.com/show_bug.cgi?id=843640 Hi Oleg, I checked the following patch and it does fix the problem on the 3.5.0+ kernel. -Will If mmap_region()-uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register(). For example, uprobe_mmap()-install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case. dup_mmap()-uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first. Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap(). Reported-by: William Cohen wco...@redhat.com Signed-off-by: Oleg Nesterov o...@redhat.com Cc: sta...@vger.kernel.org # v3.5 --- kernel/fork.c |4 ++-- mm/mmap.c |5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..54bb88a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); } /* a new mm has just been created */ arch_dup_mmap(oldmm, mm); diff --git a/mm/mmap.c b/mm/mmap.c index 4fe2697..f25fd3f 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1355,9 +1355,8 @@ out: } else if ((flags MAP_POPULATE) !(flags MAP_NONBLOCK)) make_pages_present(addr, addr + len); - if (file uprobe_mmap(vma)) - /* matching probes but cannot insert */ - goto unmap_and_free_vma; + if (file) + uprobe_mmap(vma); return addr; -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
On 07/28, Oleg Nesterov wrote: > > https://bugzilla.redhat.com/show_bug.cgi?id=843640 > > If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path > does unmap_region() but does not remove the soon-to-be-freed vma > from rb tree (actually there are more problems). Just in case... Ingo, this is orthogonal to other pending changes I sent. I think 3.6 (and 3.5-stable) needs this fix. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails
https://bugzilla.redhat.com/show_bug.cgi?id=843640 If mmap_region()->uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register(). For example, uprobe_mmap()->install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case. dup_mmap()->uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first. Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap(). Reported-by: William Cohen Signed-off-by: Oleg Nesterov Cc: # v3.5 --- kernel/fork.c |4 ++-- mm/mmap.c |5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..54bb88a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file && uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); } /* a new mm has just been created */ arch_dup_mmap(oldmm, mm); diff --git a/mm/mmap.c b/mm/mmap.c index 4fe2697..f25fd3f 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1355,9 +1355,8 @@ out: } else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK)) make_pages_present(addr, addr + len); - if (file && uprobe_mmap(vma)) - /* matching probes but cannot insert */ - goto unmap_and_free_vma; + if (file) + uprobe_mmap(vma); return addr; -- 1.5.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
https://bugzilla.redhat.com/show_bug.cgi?id=843640 If mmap_region()-uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Perhaps we could do do_munmap() + return in this case, but in fact it is simply wrong to abort if uprobe_mmap() fails. Until at least we move the !UPROBE_COPY_INSN code from install_breakpoint() to uprobe_register(). For example, uprobe_mmap()-install_breakpoint() can fail if the probed insn is not supported (remember, uprobe_register() succeeds if nobody mmaps inode/offset), mmap() should not fail in this case. dup_mmap()-uprobe_mmap() is wrong too by the same reason, fork() can race with uprobe_register() and fail for no reason if it wins the race and does install_breakpoint() first. Change mmap_region() and dup_mmap() to ignore the error code from uprobe_mmap(). Reported-by: William Cohen wco...@redhat.com Signed-off-by: Oleg Nesterov o...@redhat.com Cc: sta...@vger.kernel.org # v3.5 --- kernel/fork.c |4 ++-- mm/mmap.c |5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index ab5211b..54bb88a 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -454,8 +454,8 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm) if (retval) goto out; - if (file uprobe_mmap(tmp)) - goto out; + if (file) + uprobe_mmap(tmp); } /* a new mm has just been created */ arch_dup_mmap(oldmm, mm); diff --git a/mm/mmap.c b/mm/mmap.c index 4fe2697..f25fd3f 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1355,9 +1355,8 @@ out: } else if ((flags MAP_POPULATE) !(flags MAP_NONBLOCK)) make_pages_present(addr, addr + len); - if (file uprobe_mmap(vma)) - /* matching probes but cannot insert */ - goto unmap_and_free_vma; + if (file) + uprobe_mmap(vma); return addr; -- 1.5.5.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] uprobes: mmap_region() corrupts mm-mm_rb if uprobe_mmap() fails
On 07/28, Oleg Nesterov wrote: https://bugzilla.redhat.com/show_bug.cgi?id=843640 If mmap_region()-uprobe_mmap() fails, unmap_and_free_vma path does unmap_region() but does not remove the soon-to-be-freed vma from rb tree (actually there are more problems). Just in case... Ingo, this is orthogonal to other pending changes I sent. I think 3.6 (and 3.5-stable) needs this fix. Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
