Re: [PATCH v3 3/3] kgdb: Add command linux.vmcoreinfo to kgdb

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Stephen Brennan <stephen.s.brennan@oracle.com> writes:

> Amal Raj T <tjarlama@gmail.com> writes:
>
>> From: Amal Raj T <amalrajt@meta.com>
>>
>> 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.

After checking in with some helpful members of the GDB mailing list[1],
it sounds like the answer here is to use the qXfer command linked above.
Essentially, the format would be like this:

  Request: qXfer:vmcoreinfo:read::OFFSET,LENGTH

  Reply:
    'm [DATA]' - DATA read from the target OFFSET. More data may be
       present. The amount of data may be less than LENGTH.
    'l [DATA]' - DATA read from the target offset. There is no more data
       to be read. The amount of data may be less than LENGTH.
    'l' - There is no data at that offset

The DATA would still be encoded in the same escaped binary format
implemented in patch 1. The useful difference here is that we would not
need to expand remcom_out_buffer. We simply reply with as much data as
requested, or as much as we can fit into the output buffer, whichever is
smaller. The client will use multiple requests until the entire data is
read.

So patch 1 would need to be updated to be aware of remaining space in
the buffer, and it would need to do partial reads. Since the escaping
scheme rarely needs to escape characters (especially for vmcoreinfo), we
can optimistically fill the whole buffer (not just half) and then we
could determine the amount of escapes and do the escaping & shifting all
at once. Something like this:

int kgdb_mem2ebin(char *mem, char *buf, int count, int cap)
{
	int err, i, esc = 0;

	err = copy_from_kernel_nofault(buf, mem, min(count, cap));
	if (err)
		return err;

	/* Some characters need to be escaped, but it's uncommon.
	 * Count the number of escaped characters and then perform
	 * the escaping in reverse. */
	for (i = 0; i < count && i + esc < cap; i++) {
		if (buf[i] == '}' || buf[i] == '#' || buf[i] == '*' || buf[i] == '#') {
			if (i + esc + 1 == cap) {
				/* Skip characters that need escaping when we only have
				 * one byte of capacity remaining. */
				buf[i] = '\0';
				break;
			} else {
				esc += 1;
			}
		}
	}
	count = i;
	i -= 1;
	while (esc > 0) {
		char c = buf[i];
		if (c == '}' || c == '#' || c == '*' || c == '#') {
			buf[i + esc] = c ^ 0x20;
			esc -= 1;
			buf[i + esc] = '}';
		} else {
			buf[i + esc] = c;
		}
		i -= 1;
	}
	return count;
}

[1]: https://inbox.sourceware.org/gdb/87y0zds39y.fsf@oracle.com/T/#t