> > 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/
