Re: [PATCH] uprobes: mmap_region() corrupts mm->mm_rb if uprobe_mmap() fails

[Oleg Nesterov <oleg@redhat.com>]

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.