Re: [PATCH 1/3] linux-user: Allow gdbstub to ignore page protection

[Richard Henderson <richard.henderson@linaro.org>]

On 1/10/24 06:39, Ilya Leoshkevich wrote:
> On Wed, 2024-01-10 at 04:42 +1100, Richard Henderson wrote:
>> On 1/9/24 10:34, Ilya Leoshkevich wrote:
>>> gdbserver ignores page protection by virtue of using
>>> /proc/$pid/mem.
>>> Teach qemu gdbstub to do this too. This will not work if /proc is
>>> not
>>> mounted; accept this limitation.
>>>
>>> One alternative is to temporarily grant the missing PROT_* bit, but
>>> this is inherently racy. Another alternative is self-debugging with
>>> ptrace(POKE), which will break if QEMU itself is being debugged - a
>>> much more severe limitation.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    cpu-target.c | 55 ++++++++++++++++++++++++++++++++++++++---------
>>> -----
>>>    1 file changed, 40 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/cpu-target.c b/cpu-target.c
>>> index 5eecd7ea2d7..69e97f78980 100644
>>> --- a/cpu-target.c
>>> +++ b/cpu-target.c
>>> @@ -406,6 +406,15 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr
>>> addr,
>>>        vaddr l, page;
>>>        void * p;
>>>        uint8_t *buf = ptr;
>>> +    int ret = -1;
>>> +    int mem_fd;
>>> +
>>> +    /*
>>> +     * Try ptrace first. If /proc is not mounted or if there is a
>>> different
>>> +     * problem, fall back to the manual page access. Note that,
>>> unlike ptrace,
>>> +     * it will not be able to ignore the protection bits.
>>> +     */
>>> +    mem_fd = open("/proc/self/mem", is_write ? O_WRONLY :
>>> O_RDONLY);
>>
>> Surely this is the unlikely fallback, and you don't need to open
>> unless the page is
>> otherwise inaccessible.
> 
> Ok, I can move this under (flags & PAGE_*) checks.
> 
>> I see no handling for writes to pages that contain TranslationBlocks.
> 
> Sorry, I completely missed that. I'm currently experimenting with the
> following:
> 
> 	/*
> 	 * If there is a TranslationBlock and we weren't bypassing
> host
> 	 * page protection, the memcpy() above would SEGV, ultimately
> 	 * leading to page_unprotect(). So invalidate the translations
> 	 * manually. Both invalidation and pwrite() must be under
> 	 * mmap_lock() in order to prevent the creation of another
> 	 * TranslationBlock in between.
> 	 */
> 	mmap_lock();
> 	tb_invalidate_phys_page(page);
> 	written = pwrite(fd, buf, l, (off_t)g2h_untagged(addr));

I would use here tb_invalidate_phys_range(addr, addr + l - 1),
but otherwise, it looks good.


r~

> 	mmap_unlock();
> 
> Does that look okay?
> 
> [...]