> OK, lets start with dup_mmap:
>
> // retval == 0
>
> if (file && uprobe_mmap(tmp))
> goto out;
>
> out:
> up_write(&mm->mmap_sem);
> flush_tlb_mm(oldmm);
> up_write(&oldmm->mmap_sem);
> return retval;
>
> Given that retval == 0, what do you think dup_mmap() returns if
> uprobe_mmap() fails? And note that we didn't copy all vmas.
> OK, at least this can't crash (afaics), and easy to fix.
>
>
> 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.
diff --git a/kernel/fork.c b/kernel/fork.c
index f00e319..78bfd94 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -456,10 +456,10 @@ static int dup_mmap(struct mm_struct *mm, struct
mm_struct *oldmm)
if (tmp->vm_ops && tmp->vm_ops->open)
tmp->vm_ops->open(tmp);
- if (retval)
- goto out;
+ if (!retval && file)
+ retval = uprobe_mmap(tmp);
- if (file && uprobe_mmap(tmp))
+ if (retval)
goto out;
}
/* a new mm has just been created */
diff --git a/mm/mmap.c b/mm/mmap.c
index 4fe2697..91d36fb 100644
--- 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;
+ }
return addr;
Basically, I am setting the return value of uprobe_mmap() so that
mmap_region and do_fork() fail. This still needs the fix in
uprobe_mmap() to ignore unsupported instructions.
I am completely _okay_ with not setting the return values as proposed by you.
Just that setting return values and the fix in uprobe_mmap() might help
in minor issues
- We either probe all probes where possible or intimate to the user that
the requested operation wasnt successful.
- If valid probes follow a probe with invalid instruction, we still
allow valid probes.
- If get_user_pages()/set_swbp() fail because of genuine reasons like
ENOMEM, then we dont retry to place probes on the subsequent vmas.
--
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/
