Re: [PATCH] uprobes: Skip breakpoint installation on non executable vmas

[Oleg Nesterov <oleg@redhat.com>]

On 08/05, Sumanth Korikkar wrote:
>
> bpftrace  -e 'usdt:./testprogs/usdt_semaphore_test:tracetest:testprobe {
> printf("%s\n", str(arg1) ); exit(); }'

I am hoping that Andrii and Jiri (cc'ed) can take a look, I know nothing
about usdt... And TBH, I don't even know what RELRO is ;)

Let me ask a couple of questions for now.

> expects a semaphore increment of 1, but semaphore gets double incremented
>
> Test program:
> https://github.com/bpftrace/bpftrace/blob/master/tests/testprogs/usdt_semaphore_test.c

Perhaps you can provide the test-case which I could compile on my
machine without libbpf-usdt/usdt.h?

And can you explain what the bpftrace cmd above actually does? I mean,
where does it put the uprobe? I guess the ref_ctr_offset argument of
uprobe_register() refers to USDT_DEFINE_SEMA() in that test-case...

> Reason: .text mapping and RELRO mapping resolve to the same page aligned
> file offset 0
> 01000000-01001000 r-xp 00000000 5e:01 usdt_semaphore_test (.text)
> 01001000-01002000 r--p 00000000 5e:01 usdt_semaphore_test (RELRO)
> 01002000-01003000 rw-p 00001000 5e:01 usdt_semaphore_test (semaphore)
>
> valid_vma() currently accepts both mappings (which contains executable
> text and RELRO mapping) during uprobe registration, since both have
> VM_MAYEXEC set. This causes register_for_each_vma() to call
> install_breakpoint() twice for the same underlying uprobe offset in the
> process.  This means, update_ref_ctr() is called twice for the same
> process, so a usdt semaphore is incremented from 0 to 2.

So, 2 vmas map the same binary, install_breakpoint() is called twice.
But, the 2nd install_breakpoint() -> ... -> uprobe_write() should see
that the original insn was already replaced by int3, in this case
verify_opcode() returns 0 and uprobe_write() should do nothing.

And, if this uprobe was optimized before the 2nd install_breakpoint(),
uprobe_write() won't be called.

Hmm.

> Installing a breakpoint for mapping without VM_EXEC and
> updating usdt reference counter in that case is not useful.
>
> Skip non VM_EXEC mappings in install_breakpoint(). This fixes semaphore
> double increment as shown in the above usecase.
>
> Signed-off-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
> ---
>  kernel/events/uprobes.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 6300b216012c..9e0bbc3cf401 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1155,6 +1155,9 @@ static int install_breakpoint(struct uprobe *uprobe, struct vm_area_struct *vma,
>  	bool first_uprobe;
>  	int ret;
>
> +	if (!(vma->vm_flags & VM_EXEC))
> +		return 0;
> +

Well, but then it makes more sense to change valid_vma() to nack the
non VM_EXEC mappings ?

Oleg.