| Message ID | 20240108233821.201325-2-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user: Allow gdbstub to ignore page protection | expand |
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. I see no handling for writes to pages that contain TranslationBlocks. r~ > > while (len > 0) { > page = addr & TARGET_PAGE_MASK; > @@ -413,22 +422,33 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, > if (l > len) > l = len; > flags = page_get_flags(page); > - if (!(flags & PAGE_VALID)) > - return -1; > + if (!(flags & PAGE_VALID)) { > + goto out_close; > + } > if (is_write) { > - if (!(flags & PAGE_WRITE)) > - return -1; > + if (mem_fd == -1 || > + pwrite(mem_fd, ptr, len, (off_t)g2h_untagged(addr)) != len) { > + if (!(flags & PAGE_WRITE)) { > + goto out_close; > + } > + /* XXX: this code should not depend on lock_user */ > + p = lock_user(VERIFY_WRITE, addr, l, 0); > + if (!p) { > + goto out_close; > + } > + memcpy(p, buf, l); > + unlock_user(p, addr, l); > + } > + } else if (mem_fd == -1 || > + pread(mem_fd, ptr, len, (off_t)g2h_untagged(addr)) != len) { > + if (!(flags & PAGE_READ)) { > + goto out_close; > + } > /* XXX: this code should not depend on lock_user */ > - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) > - return -1; > - memcpy(p, buf, l); > - unlock_user(p, addr, l); > - } else { > - if (!(flags & PAGE_READ)) > - return -1; > - /* XXX: this code should not depend on lock_user */ > - if (!(p = lock_user(VERIFY_READ, addr, l, 1))) > - return -1; > + p = lock_user(VERIFY_READ, addr, l, 1); > + if (!p) { > + goto out_close; > + } > memcpy(buf, p, l); > unlock_user(p, addr, 0); > } > @@ -436,7 +456,12 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, > buf += l; > addr += l; > } > - return 0; > + ret = 0; > +out_close: > + if (mem_fd != -1) { > + close(mem_fd); > + } > + return ret; > } > #endif >
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)); mmap_unlock(); Does that look okay? [...]
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? > > [...]
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); while (len > 0) { page = addr & TARGET_PAGE_MASK; @@ -413,22 +422,33 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, if (l > len) l = len; flags = page_get_flags(page); - if (!(flags & PAGE_VALID)) - return -1; + if (!(flags & PAGE_VALID)) { + goto out_close; + } if (is_write) { - if (!(flags & PAGE_WRITE)) - return -1; + if (mem_fd == -1 || + pwrite(mem_fd, ptr, len, (off_t)g2h_untagged(addr)) != len) { + if (!(flags & PAGE_WRITE)) { + goto out_close; + } + /* XXX: this code should not depend on lock_user */ + p = lock_user(VERIFY_WRITE, addr, l, 0); + if (!p) { + goto out_close; + } + memcpy(p, buf, l); + unlock_user(p, addr, l); + } + } else if (mem_fd == -1 || + pread(mem_fd, ptr, len, (off_t)g2h_untagged(addr)) != len) { + if (!(flags & PAGE_READ)) { + goto out_close; + } /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_WRITE, addr, l, 0))) - return -1; - memcpy(p, buf, l); - unlock_user(p, addr, l); - } else { - if (!(flags & PAGE_READ)) - return -1; - /* XXX: this code should not depend on lock_user */ - if (!(p = lock_user(VERIFY_READ, addr, l, 1))) - return -1; + p = lock_user(VERIFY_READ, addr, l, 1); + if (!p) { + goto out_close; + } memcpy(buf, p, l); unlock_user(p, addr, 0); } @@ -436,7 +456,12 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, buf += l; addr += l; } - return 0; + ret = 0; +out_close: + if (mem_fd != -1) { + close(mem_fd); + } + return ret; } #endif
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(-)