Amal Raj T <tjarlama@xxxxxxxxx> writes: > From: Amal Raj T <amalrajt@xxxxxxxx> > > Add a new query `linux.vmcoreinfo` to kgdb that returns > vmcoreinfo to the client using the mem2ebin encoding. > Maximum size of output buffer is set to 2x the maximum > size of VMCOREINFO_BYTES (kgdb_mem2ebin() requires 1x > for the temporary copy plus another 1x (max) for the > escaped data). > > Link: https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet > --- > kernel/debug/gdbstub.c | 18 ++++++++++++++---- > lib/Kconfig.kgdb | 1 + > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c > index f88e21d5502a..f2c80bd368e2 100644 > --- a/kernel/debug/gdbstub.c > +++ b/kernel/debug/gdbstub.c > @@ -30,20 +30,21 @@ > #include <linux/kgdb.h> > #include <linux/kdb.h> > #include <linux/serial_core.h> > +#include <linux/string.h> > #include <linux/reboot.h> > #include <linux/uaccess.h> > #include <asm/cacheflush.h> > #include <linux/unaligned.h> > +#include <linux/vmcore_info.h> > #include "debug_core.h" > > #define KGDB_MAX_THREAD_QUERY 17 > > /* Our I/O buffers. */ > static char remcom_in_buffer[BUFMAX]; > -static char remcom_out_buffer[BUFMAX]; > +static char remcom_out_buffer[MAX(VMCOREINFO_BYTES * 2, BUFMAX)]; Looking at the code added to gdb_cmd_query(), the actual maximum size of the response is VMCOREINFO_BYTES * 2 + 1, to account for the 'Q' character. This is a large buffer, for most architectures it would be 8193 bytes, compared to the current 2048 or 4096. The more I look at this, the more concerned I am that we're going about this wrong. GDB has packet types which allow reading uninterpreted bytes of OS-related data, and they allow specifying a requested offset and length: https://sourceware.org/gdb/current/onlinedocs/gdb.html/General-Query-Packets.html#qXfer-read This would ensure that we could break up the data into multiple smaller packets, which may go with the protocol design better. I can't find any documented maximum packet size, but in the GDB remote.c code, I see it being initialized as small as 400. I'm aware that I come up with the "qlinux.vmcoreinfo" idea so I'm the one who should be chastised for missing this potential limitation if it does exist. I think I'll need to go into the GDB code and play around with this more, and also share this idea with the binutils people who I probably should have consulted in the first place. > static int gdbstub_use_prev_in_buf; > static int gdbstub_prev_in_buf_pos; > - > /* Storage for the registers, in GDB format. */ > static unsigned long gdb_regs[(NUMREGBYTES + sizeof(unsigned long) - 1) / > sizeof(unsigned long)]; > @@ -292,8 +293,8 @@ char *kgdb_mem2ebin(char *mem, char *buf, int count) > } else { > *buf++ = c; > } > - count -= 1; > - tmp += 1; > + count--; > + tmp++; I don't think this update goes with the rest of this patch. If you'd prefer the "++" and "--" syntax, then please incorporate that into patch 1. > } > > return buf; > @@ -777,6 +778,15 @@ static void gdb_cmd_query(struct kgdb_state *ks) > *(--ptr) = '\0'; > break; > > + case 'l': > + if (strncmp(remcom_in_buffer + 1, "linux.vmcoreinfo", 17) == > + 0) { > + remcom_out_buffer[0] = 'Q'; > + kgdb_mem2ebin(vmcoreinfo_data, remcom_out_buffer + 1, > + vmcoreinfo_size); Answering my question on patch 1: vmcoreinfo_size does not include the NUL byte. So when we get to put_packet(), the loop would continue into the staged copy of the data. Effectively, some portion of the original (unescaped) vmcoreinfo would get output after the escaped vmcoreinfo. Or at least, that's how I read the code. Has this occurred in testing, or is there something I misunderstood in this analysis? > + } > + break; > + > case 'C': > /* Current thread id */ > strcpy(remcom_out_buffer, "QC"); > diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb > index 537e1b3f5734..012529eee79e 100644 > --- a/lib/Kconfig.kgdb > +++ b/lib/Kconfig.kgdb > @@ -12,6 +12,7 @@ menuconfig KGDB > bool "KGDB: kernel debugger" > depends on HAVE_ARCH_KGDB > depends on DEBUG_KERNEL > + select VMCORE_INFO I don't know enough to know whether this would be an issue in practice, but I'd guess that some people may use kgdb without VMCORE_INFO, and they may be more likely to be in an embedded application where they track memory usage carefully, and where the vmcoreinfo may be less useful to them. It may be wise to avoid selecting VMCORE_INFO, but instead fall back to a stub implementation that returns an error packet indicating that there is no data. Thanks, Stephen > help > If you say Y here, it will be possible to remotely debug the > kernel using gdb. It is recommended but not required, that > -- > 2.43.5
