Messages in this thread | | | Date | Fri, 3 Aug 2012 15:38:25 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] uprobes: Ignore unsupported instructions in uprobe_mmap |
| |
On 08/03, Srikar Dronamraju wrote: > > > But mmap_region() is worse, much worse. It simply can _not_ fail > > after uprobe_mmap (of course, I am not saying this is unfixable) > > without the crash. And note that the crash is "delayed". And btw, > > like dup_mmap(), mmap_region() doesn't return the error too. > > > > Srikar, I strongly believe this horror must not exist. Either > > we should teach mmap_region() and dup_mmap() (and vma_adjust!) > > to fail correctly, or we should ignore the error code. > > > > It is that simple, isn't it? > > I think you would have thought of this approach already but just wanted > to check if below is fine with you. > > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1355,9 +1355,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > } else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK)) > make_pages_present(addr, addr + len); > > - if (file && uprobe_mmap(vma)) > + if (file) { > + error = uprobe_mmap(vma) > /* matching probes but cannot insert */ > - goto unmap_and_free_vma; > + if (error) > + goto unmap_and_free_vma;
No, this is wrong.
Please look at the code under unmap_and_free_vma. The main part is unmap_region(), but this does NOT remove vma from mm->mm_rb and then this vma is kmem_cache_free'ed. IOW, this leaves the freed object in rb tree.
Afaics, there are other things this error path doesn't do but this is how William noticed the bug (kernel hang). I don't think the fix is suitable for stable.
Srikar, can't we make these fixes on top of my simple patch which fixes the easy-to-trigger kernel crashes/hangs?
If yes, may be you can ack that patch for Ingo?
And yes, uprobe_mmap() needs changes too. But can't we do this a bit later? Currently uprobes_state.count is very wrong, it simply does not count uprobes correctly even in the very simple cases.
Oleg.
| |