Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> On Mon, Nov 14, 2016 at 02:10:32PM -0500, Antoine Tremblay wrote:
>>> 
>>> > I do something slightly differently in V3.  In my
>>> > "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
>>> > for needed lwps in two places, linux_resume and proceed_all_lwps, which
>>> > isn't ideal.
>>> >
>>> > After the chat with Pedro, we don't need to stop all threads when inserting
>>> > reinsert breakpoint, so we can move the breakpoint installation further
>>> > down to linux_resume_one_thread and proceed_one_lwp.
>>> 
>>> I'm following up on random SIGILL/SIGSEGV when using software single stepping/
>>> range stepping with GDBServer on ARM.
>>> 
>>> And I can't see why we don't need to stop all threads when inserting
>>> reinsert breakpoint.
>>> 
>>> Since linux_resume will call:
>>> 
>>> find_inferior (&all_threads, linux_resume_one_thread,
>>>   &leave_all_stopped);
>>> 
>>> This will start one thread after the other. So for example if thread 3
>>> has a single step breakpoint to install this will start thread 1, then
>>> thread 2 and just modify the program's memory to install reinsert
>>> breakpoints on thread 3 with thread 1 and 2 running.
>>> 
>>> Thus leading to thread 1 or 2 executing invalid memory, thus the SIGILL
>>> random problems...
>>
>> Single-step breakpoint is thread specific, so we don't need to stop
>> other threads when inserting one for a specific thread.  Given the
>> example above, we insert single-step breakpoint for thread 3 on address
>> A, if thread 1 goes through address A, but doesn't hit the breakpoint,
>> IOW, thread 1 still sees the original instruction, that is nothing wrong,
>> right?  We don't expect thread 1 hits that breakpoint for thread 3 anyway.
>> If thread 1 hits the breakpoint (IOW, thread 1 sees the breakpoint
>> instruction), GDBserver just handles that SIGTRAP, and it
>> has already know that there is a breakpoint on address A.
>>
>
> Yes that would be fine if the instruction write was atomic...
>
>> Thread 1 either sees the original instruction on address A or the
>> breakpoint instruction.  Unless ptrace read/write 32-bit is not
>> atomic, IOW, partial ptrace write result is visible to other
>> threads, I don't see why we get SIGILL here.
>
> I think this is the problem, ptrace read/write doesn't seem to be
> atomic, and thread 1 sees some half written memory. (Given that we get
> SIGILL/SIGSEGV issues)
>
> Did you have any reference suggesting it was atomic ?
>
> While testing it seems to be atomic for 32bit writes but in thumb mode
> with a 16 byte write, it is not.
>
> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
> I'll have to dig in the ptrace code I guess.
>
> The SIGILL/SIGSEGV do go away if GDBServer stops the threads...
>

After some digging on the atomicity of replacing instructions I found:

A3.5.4
Concurrent modification and execution of instructions
The ARMv7 architecture limits the set of instructions that can be executed by one thread of execution as they are
being modified by another thread of execution without requiring explicit synchronization.
Except for the instructions identified in this section, the effect of the concurrent modification and execution of an
instruction is UNPREDICTABLE .

I found something similar with kprobes on aarch64 see:

linux/arch/arm64/kernel/insn.c
int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)

This checks if the instruction is one that is safe to hot patch, and if
not it basically stops all execution on the machine except for the task
of replacing this instruction.

Given that GDB does not distinguish between the instructions I think
it's safer to stop all the threads before modifiing the instruction.

>>
>> Note that we stop all threads when we remove single-step breakpoints
>> because we want no thread sees single-step breakpoint in memory from
>> their point of view afterwards.
>
> Yes that's fine.