[PATCH 0/3] Add a new command in kgdb for vmcoreinfo

[PATCH 0/3] Add a new command in kgdb for vmcoreinfo

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

This patch set introduces a new custom query packet,
`qlinux.vmcoreinfo`, to kgdb. This packet retrieves kernel's
vmcoreinfo text, which contains crucial debugging information
such as the KASLR offset, kernel release, `phys_base` etc.

This implementation includes a new binary-encoder (`mem2ebin`)
instead of the existing hex-encoder (`mem2hex`) to reduce the
amount of data sent over the wire.

These changes also move the LF -> CRLF replacement logic from
serial drivers to KDB, since KDB is the only user of this
logic, and the existing replacement results in incorrect
checksums generated by KGDB.

Link:
- https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet

Amal Raj T (3):
  kgdb: Add kgdb_mem2ebin function for converting memory to binary
    format
  serial: Move LF -> CRLF replacement from serial console to kdb
  kgdb: Add command linux.vmcoreinfo to kgdb

 drivers/tty/serial/serial_core.c |  4 ----
 include/linux/kgdb.h             |  1 +
 kernel/debug/gdbstub.c           | 41 +++++++++++++++++++++++++++++++-
 kernel/debug/kdb/kdb_io.c        |  2 ++
 lib/Kconfig.kgdb                 |  1 +
 5 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.43.5

[PATCH 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

Add a new function kgdb_mem2ebin that converts memory
to binary format, escaping special characters
('$', '#', and '}'). kgdb_mem2ebin function ensures
that memory data is properly formatted and escaped
before being sent over the wire. Additionally, this
function reduces the amount of data exchanged between
debugger compared to hex.

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 include/linux/kgdb.h   |  1 +
 kernel/debug/gdbstub.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 76e891ee9e37..fa3cf38a14de 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
 
 extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
 extern char *kgdb_mem2hex(char *mem, char *buf, int count);
+extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
 extern int kgdb_hex2mem(char *buf, char *mem, int count);
 
 extern int kgdb_isremovedbreak(unsigned long addr);
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index f625172d4b67..6198d2eb49c4 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -257,6 +257,37 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
 	return buf;
 }
 
+/*
+ * Convert memory to binary format for GDB remote protocol
+ * transmission, escaping special characters ($, #, and }).
+ */
+char *kgdb_mem2ebin(char *mem, char *buf, int count)
+{
+	char *tmp;
+	int err;
+
+	tmp = buf + count;
+
+	err = copy_from_kernel_nofault(tmp, mem, count);
+	if (err)
+		return NULL;
+	while (count > 0) {
+		unsigned char c = *tmp;
+
+		if (c == 0x7d || c == 0x23 || c == 0x24) {
+			*buf++ = 0x7d;
+			*buf++ = c ^ 0x20;
+		} else {
+			*buf++ = c;
+		}
+		count -= 1;
+		tmp += 1;
+	}
+	*buf = 0;
+
+	return buf;
+}
+
 /*
  * Convert the hex array pointed to by buf into binary to be placed in
  * mem.  Return a pointer to the character AFTER the last byte
-- 
2.43.5

[PATCH 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

The current implementation of `poll_put_char` in the serial console driver
performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
is the only user of `poll_put_char`, this patch moves the LF -> CRLF
replacement logic to kdb.

Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 drivers/tty/serial/serial_core.c | 4 ----
 kernel/debug/kdb/kdb_io.c        | 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74fa02b23772..ed18492b7f8f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2142,8 +2142,6 @@ void uart_console_write(struct uart_port *port, const char *s,
 	unsigned int i;
 
 	for (i = 0; i < count; i++, s++) {
-		if (*s == '\n')
-			putchar(port, '\r');
 		putchar(port, *s);
 	}
 }
@@ -2738,8 +2736,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	if (!port)
 		return;
 
-	if (ch == '\n')
-		port->ops->poll_put_char(port, '\r');
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 6a77f1c779c4..43a7c8ad741a 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	len = msg_len;
 
 	while (len--) {
+		if (*cp == '\n')
+			dbg_io_ops->write_char('\r');
 		dbg_io_ops->write_char(*cp);
 		cp++;
 	}
-- 
2.43.5

Re: [PATCH 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Daniel Thompson]

On Tue, Dec 10, 2024 at 05:34:47AM -0800, Amal Raj T wrote:
> From: Amal Raj T <amalrajt@meta.com>
>
> The current implementation of `poll_put_char` in the serial console driver
> performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> replacement logic to kdb.

This description only explains why it is safe to change
uart_poll_put_char() but...



> Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
>
> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  drivers/tty/serial/serial_core.c | 4 ----
>  kernel/debug/kdb/kdb_io.c        | 2 ++
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 74fa02b23772..ed18492b7f8f 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2142,8 +2142,6 @@ void uart_console_write(struct uart_port *port, const char *s,
>  	unsigned int i;
>
>  	for (i = 0; i < count; i++, s++) {
> -		if (*s == '\n')
> -			putchar(port, '\r');
>  		putchar(port, *s);
>  	}
>  }

... kgdb isn't the only user of uart_console_write() though, right?


Daniel.

Re: [PATCH 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Amal]

On Tue, Dec 10, 2024 at 3:16 PM Daniel Thompson <daniel@riscstar.com> wrote:
>
> On Tue, Dec 10, 2024 at 05:34:47AM -0800, Amal Raj T wrote:
> > From: Amal Raj T <amalrajt@meta.com>
> >
> > The current implementation of `poll_put_char` in the serial console driver
> > performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> > is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> > replacement logic to kdb.
>
> This description only explains why it is safe to change
> uart_poll_put_char() but...
>
>
>
> > Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
> >
> > Signed-off-by: Amal Raj T <amalrajt@meta.com>
> > ---
> >  drivers/tty/serial/serial_core.c | 4 ----
> >  kernel/debug/kdb/kdb_io.c        | 2 ++
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 74fa02b23772..ed18492b7f8f 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2142,8 +2142,6 @@ void uart_console_write(struct uart_port *port, const char *s,
> >       unsigned int i;
> >
> >       for (i = 0; i < count; i++, s++) {
> > -             if (*s == '\n')
> > -                     putchar(port, '\r');
> >               putchar(port, *s);
> >       }
> >  }
>
> ... kgdb isn't the only user of uart_console_write() though, right?
Yes, the replacement should be only for uart_poll_put_char, fixed this in v2
>
>
> Daniel.

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

[Amal Raj T]

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 3X the maximum
size of VMCOREINFO_BYTES (kgdb_mem2ebin() requires 1x
for the temporary copy plus up to 2x for the escaped
data).

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 kernel/debug/gdbstub.c | 10 +++++++++-
 lib/Kconfig.kgdb       |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 6198d2eb49c4..5bec444fc6d3 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -34,13 +34,14 @@
 #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*3, BUFMAX)];
 static int			gdbstub_use_prev_in_buf;
 static int			gdbstub_prev_in_buf_pos;
 
@@ -768,6 +769,13 @@ static void gdb_cmd_query(struct kgdb_state *ks)
 		*(--ptr) = '\0';
 		break;
 
+	case 'l':
+		if (memcmp(remcom_in_buffer + 1, "linux.vmcoreinfo", 16) == 0) {
+			remcom_out_buffer[0] = 'Q';
+			kgdb_mem2ebin(vmcoreinfo_data, remcom_out_buffer + 1, vmcoreinfo_size);
+		}
+		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
 	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

[PATCH v2 0/3] Add a new command in kgdb for vmcoreinfo

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

This patch set introduces a new custom query packet,
`qlinux.vmcoreinfo`, to kgdb. This packet retrieves kernel's
vmcoreinfo text, which contains crucial debugging information
such as the KASLR offset, kernel release, `phys_base` etc.

This implementation includes a new binary-encoder (`mem2ebin`)
instead of the existing hex-encoder (`mem2hex`) to reduce the
amount of data sent over the wire.

These changes also move the LF -> CRLF replacement logic from
serial drivers to KDB, since KDB is the only user of this
logic, and the existing replacement results in incorrect
checksums generated by KGDB.

Link:
- https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet

v2:
Added back LF neplacement logic in `uart_console_write`

Amal Raj T (3):
  kgdb: Add kgdb_mem2ebin function for converting memory to binary
    format
  serial: Move LF -> CRLF replacement from serial console to kdb
  kgdb: Add command linux.vmcoreinfo to kgdb

 drivers/tty/serial/serial_core.c |  2 --
 include/linux/kgdb.h             |  1 +
 kernel/debug/gdbstub.c           | 41 +++++++++++++++++++++++++++++++-
 kernel/debug/kdb/kdb_io.c        |  2 ++
 lib/Kconfig.kgdb                 |  1 +
 5 files changed, 44 insertions(+), 3 deletions(-)

-- 
2.43.5

[PATCH v2 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

Add a new function kgdb_mem2ebin that converts memory
to binary format, escaping special characters
('$', '#', and '}'). kgdb_mem2ebin function ensures
that memory data is properly formatted and escaped
before being sent over the wire. Additionally, this
function reduces the amount of data exchanged between
debugger compared to hex.

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 include/linux/kgdb.h   |  1 +
 kernel/debug/gdbstub.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 76e891ee9e37..fa3cf38a14de 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
 
 extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
 extern char *kgdb_mem2hex(char *mem, char *buf, int count);
+extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
 extern int kgdb_hex2mem(char *buf, char *mem, int count);
 
 extern int kgdb_isremovedbreak(unsigned long addr);
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index f625172d4b67..6198d2eb49c4 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -257,6 +257,37 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
 	return buf;
 }
 
+/*
+ * Convert memory to binary format for GDB remote protocol
+ * transmission, escaping special characters ($, #, and }).
+ */
+char *kgdb_mem2ebin(char *mem, char *buf, int count)
+{
+	char *tmp;
+	int err;
+
+	tmp = buf + count;
+
+	err = copy_from_kernel_nofault(tmp, mem, count);
+	if (err)
+		return NULL;
+	while (count > 0) {
+		unsigned char c = *tmp;
+
+		if (c == 0x7d || c == 0x23 || c == 0x24) {
+			*buf++ = 0x7d;
+			*buf++ = c ^ 0x20;
+		} else {
+			*buf++ = c;
+		}
+		count -= 1;
+		tmp += 1;
+	}
+	*buf = 0;
+
+	return buf;
+}
+
 /*
  * Convert the hex array pointed to by buf into binary to be placed in
  * mem.  Return a pointer to the character AFTER the last byte
-- 
2.43.5

Re: [PATCH v2 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Doug Anderson]

Hi,

On Wed, Dec 11, 2024 at 7:40 AM Amal Raj T <tjarlama@gmail.com> wrote:
>
> From: Amal Raj T <amalrajt@meta.com>
>
> Add a new function kgdb_mem2ebin that converts memory
> to binary format, escaping special characters
> ('$', '#', and '}'). kgdb_mem2ebin function ensures
> that memory data is properly formatted and escaped
> before being sent over the wire. Additionally, this
> function reduces the amount of data exchanged between
> debugger compared to hex.
>
> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  include/linux/kgdb.h   |  1 +
>  kernel/debug/gdbstub.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 76e891ee9e37..fa3cf38a14de 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
>
>  extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
>  extern char *kgdb_mem2hex(char *mem, char *buf, int count);
> +extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
>  extern int kgdb_hex2mem(char *buf, char *mem, int count);
>
>  extern int kgdb_isremovedbreak(unsigned long addr);
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index f625172d4b67..6198d2eb49c4 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -257,6 +257,37 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
>         return buf;
>  }
>
> +/*
> + * Convert memory to binary format for GDB remote protocol
> + * transmission, escaping special characters ($, #, and }).

Why exactly are those characters special? What considers them special
and so why do you need to escape them? I guess you really just care
about avoiding # and $ and you're using '}' as your escape character
so you need to escape that too?

Your function comment should describe the escaping method and ideally
provide a few examples.


> + */
> +char *kgdb_mem2ebin(char *mem, char *buf, int count)

One of the two buffers seems like it should be "const", right? That
would help document which was input and which was output. I guess
"mem" is the input?

"count" should be "size_t"

Presumably there should be two counts talking about the sizes of each
buffer, or at least some documentation should be in the function
comment talking about the fact that "buf" needs to be twice the size?


> +{
> +       char *tmp;
> +       int err;
> +
> +       tmp = buf + count;

Could use a comment that the buffer needs to be 2x long to handle
escaping and that you'll use the 2nd half as a temp buffer.


> +
> +       err = copy_from_kernel_nofault(tmp, mem, count);
> +       if (err)
> +               return NULL;
> +       while (count > 0) {

If you change `count` to `size_t` the above test won't work because
it'll be unsigned. Still probably better to use `size_t`, but just a
warning that you'll have to change the condition.


> +               unsigned char c = *tmp;
> +
> +               if (c == 0x7d || c == 0x23 || c == 0x24) {
> +                       *buf++ = 0x7d;

Don't hardcode. Much better to use '}', '#', '$'


> +                       *buf++ = c ^ 0x20;
> +               } else {
> +                       *buf++ = c;
> +               }
> +               count -= 1;
> +               tmp += 1;

count--;
tmp++;

> +       }
> +       *buf = 0;

Why is the resulting buffer '\0' terminated when the source buffer
isn't? Adding this termination means that the destination buffer needs
to be 1 byte bigger, right?

...or maybe the source buffer doesn't actually have any embedded '\0'
characters and you're using this for termination to the other side? It
would be good to clarify.

In other words, if the input is 2 bytes big:
'}}'

The output buffer will be 5 bytes big:
'}]}]\0'

> +
> +       return buf;

What exactly is this return value? It points right past the end of the buffer?

You seem to just use it as an error value (NULL means error, non-NULL
means no error). Why not return an error code then?

-Doug

Re: [PATCH v2 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Doug Anderson]

Hi,

On Mon, Jan 13, 2025 at 8:09 AM Amal <tjarlama@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 12:55 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Dec 11, 2024 at 7:40 AM Amal Raj T <tjarlama@gmail.com> wrote:
> > >
> > > From: Amal Raj T <amalrajt@meta.com>
> > >
> > > Add a new function kgdb_mem2ebin that converts memory
> > > to binary format, escaping special characters
> > > ('$', '#', and '}'). kgdb_mem2ebin function ensures
> > > that memory data is properly formatted and escaped
> > > before being sent over the wire. Additionally, this
> > > function reduces the amount of data exchanged between
> > > debugger compared to hex.
> > >
> > > Signed-off-by: Amal Raj T <amalrajt@meta.com>
> > > ---
> > >  include/linux/kgdb.h   |  1 +
> > >  kernel/debug/gdbstub.c | 31 +++++++++++++++++++++++++++++++
> > >  2 files changed, 32 insertions(+)

FWIW, it looks like Stephen has already responded to your V3 and the
protocol might change a bunch. I'm not going to do a detailed review
on it at this time. I'll comment on a few of the things below, though.

One other note is that you should have done a "reply to all" when
responding to my review feedback, not just a reply to me. The
responses should be archived on the lists unless there was a specific
reason to exclude them. Adding the lists back here since I don't see
anything sensitive in your reply.


> > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > index 76e891ee9e37..fa3cf38a14de 100644
> > > --- a/include/linux/kgdb.h
> > > +++ b/include/linux/kgdb.h
> > > @@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
> > >
> > >  extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
> > >  extern char *kgdb_mem2hex(char *mem, char *buf, int count);
> > > +extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
> > >  extern int kgdb_hex2mem(char *buf, char *mem, int count);
> > >
> > >  extern int kgdb_isremovedbreak(unsigned long addr);
> > > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> > > index f625172d4b67..6198d2eb49c4 100644
> > > --- a/kernel/debug/gdbstub.c
> > > +++ b/kernel/debug/gdbstub.c
> > > @@ -257,6 +257,37 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
> > >         return buf;
> > >  }
> > >
> > > +/*
> > > + * Convert memory to binary format for GDB remote protocol
> > > + * transmission, escaping special characters ($, #, and }).
> >
> > Why exactly are those characters special? What considers them special
> > and so why do you need to escape them? I guess you really just care
> > about avoiding # and $ and you're using '}' as your escape character
> > so you need to escape that too?
> >
> > Your function comment should describe the escaping method and ideally
> > provide a few examples.
> >
> >
> Added relevant links in the commit message and commented two examples
> in code
> > > + */
> > > +char *kgdb_mem2ebin(char *mem, char *buf, int count)
> >
> > One of the two buffers seems like it should be "const", right? That
> > would help document which was input and which was output. I guess
> > "mem" is the input?
> >
> > "count" should be "size_t"
> >
> This was copied from `kgdb_mem2hex` which uses a similar template.
> Should we continue using `int` or just this function need to be migrated to
> `size_t`

In general the existing kdb/kgdb functions are not always the best
examples. I think size_t is correct here. Indeed, if you look at what
"kgdb_mem2hex" does with "count" it passes it to
copy_from_kernel_nofault() which takes a size_t.

Feel free to post a (separate) patch fixing kgdb_mem2hex().


> > Presumably there should be two counts talking about the sizes of each
> > buffer, or at least some documentation should be in the function
> > comment talking about the fact that "buf" needs to be twice the size?
> >
> >
> > > +{
> > > +       char *tmp;
> > > +       int err;
> > > +
> > > +       tmp = buf + count;
> >
> > Could use a comment that the buffer needs to be 2x long to handle
> > escaping and that you'll use the 2nd half as a temp buffer.
> >
> >
> Done, commented in v3
> > > +
> > > +       err = copy_from_kernel_nofault(tmp, mem, count);
> > > +       if (err)
> > > +               return NULL;
> > > +       while (count > 0) {
> >
> > If you change `count` to `size_t` the above test won't work because
> > it'll be unsigned. Still probably better to use `size_t`, but just a
> > warning that you'll have to change the condition.
> >
> >
> > > +               unsigned char c = *tmp;
> > > +
> > > +               if (c == 0x7d || c == 0x23 || c == 0x24) {
> > > +                       *buf++ = 0x7d;
> >
> > Don't hardcode. Much better to use '}', '#', '$'
> >
> Fixed in v3
> >
> > > +                       *buf++ = c ^ 0x20;
> > > +               } else {
> > > +                       *buf++ = c;
> > > +               }
> > > +               count -= 1;
> > > +               tmp += 1;
> >
> > count--;
> > tmp++;
> >
> Fixed in v3
> > > +       }
> > > +       *buf = 0;
> >
> > Why is the resulting buffer '\0' terminated when the source buffer
> > isn't? Adding this termination means that the destination buffer needs
> > to be 1 byte bigger, right?
> >
> > ...or maybe the source buffer doesn't actually have any embedded '\0'
> > characters and you're using this for termination to the other side? It
> > would be good to clarify.
> >
> > In other words, if the input is 2 bytes big:
> > '}}'
> >
> > The output buffer will be 5 bytes big:
> > '}]}]\0'
> >
> Don't think null termination is required in this case.
> In this case, the `gdb_serial_stub` already initializes the buffer with `\0`,
> and `put_packet` iterates over the buffer until the first occurrence of \0.
> This works for `vmcoreinfo` query. But for input buffers with additional
> `\0` characters need to be escaped, to make sure `put_packet` parsed the
> entire buffer.
> > > +
> > > +       return buf;
> >
> > What exactly is this return value? It points right past the end of the buffer?
> >
> > You seem to just use it as an error value (NULL means error, non-NULL
> > means no error). Why not return an error code then?
> >
> Other encoders also return a `char *`. However there is no error handling
> anywhere, the return value is discarded at the caller side.

As per above, existing kdb/kgdb code isn't always the best example to
follow. Personally I'd rather see an error code returned unless
there's a good reason not to.

If you really want to return a pointer like this, a bare minimum would
be to document it. The kgdb_mem2hex() function you pointed to _does_
at least document what is returned.

-Doug

Re: [PATCH v2 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Daniel Thompson]

On Wed, Dec 11, 2024 at 07:39:53AM -0800, Amal Raj T wrote:
> From: Amal Raj T <amalrajt@meta.com>
>
> Add a new function kgdb_mem2ebin that converts memory
> to binary format, escaping special characters
> ('$', '#', and '}').

What about the '*' character?

The gdb docs say "Responses sent by the stub must also escape 0x2a
(ASCII ‘*’), so that it is not interpreted as the start of a run-length
encoded sequence".


> kgdb_mem2ebin function ensures
> that memory data is properly formatted and escaped
> before being sent over the wire. Additionally, this
> function reduces the amount of data exchanged between
> debugger compared to hex.
>
> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  include/linux/kgdb.h   |  1 +
>  kernel/debug/gdbstub.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 76e891ee9e37..fa3cf38a14de 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
>
>  extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
>  extern char *kgdb_mem2hex(char *mem, char *buf, int count);
> +extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
>  extern int kgdb_hex2mem(char *buf, char *mem, int count);
>
>  extern int kgdb_isremovedbreak(unsigned long addr);
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index f625172d4b67..6198d2eb49c4 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -257,6 +257,37 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
>  	return buf;
>  }
>
> +/*
> + * Convert memory to binary format for GDB remote protocol
> + * transmission, escaping special characters ($, #, and }).
> + */

It would be good to include a link to the following in this comment:
https://sourceware.org/gdb/current/onlinedocs/gdb.html/Overview.html#Binary-Data

Doug already mentioned putting buffer size expectations in this comment.
Maybe also mention that the worst case packing is identical to
kgdb_mem2hex() (e.g. 2*count + 1).


Daniel.

[PATCH v2 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

The current implementation of `poll_put_char` in the serial console driver
performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
is the only user of `poll_put_char`, this patch moves the LF -> CRLF
replacement logic to kdb.

Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 drivers/tty/serial/serial_core.c | 2 --
 kernel/debug/kdb/kdb_io.c        | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74fa02b23772..8e702f3deffb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2738,8 +2738,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	if (!port)
 		return;
 
-	if (ch == '\n')
-		port->ops->poll_put_char(port, '\r');
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 6a77f1c779c4..43a7c8ad741a 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	len = msg_len;
 
 	while (len--) {
+		if (*cp == '\n')
+			dbg_io_ops->write_char('\r');
 		dbg_io_ops->write_char(*cp);
 		cp++;
 	}
-- 
2.43.5

Re: [PATCH v2 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Doug Anderson]

Hi,

On Wed, Dec 11, 2024 at 7:40 AM Amal Raj T <tjarlama@gmail.com> wrote:
>
> From: Amal Raj T <amalrajt@meta.com>
>
> The current implementation of `poll_put_char` in the serial console driver
> performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> replacement logic to kdb.
>
> Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
>
> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  drivers/tty/serial/serial_core.c | 2 --
>  kernel/debug/kdb/kdb_io.c        | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

Looks reasonable. If someone comes out of the woodwork and says that
this breaks them then we can try to figure out a solution as talked
about previously [1]. It would be nice to include a link to the
previous conversation in your comment message...

With the reference to the previous conversation:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[1] https://lore.kernel.org/r/20241115144933.GB4408@aspen.lan

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

[Amal Raj T]

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 3X the maximum
size of VMCOREINFO_BYTES (kgdb_mem2ebin() requires 1x
for the temporary copy plus up to 2x for the escaped
data).

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 kernel/debug/gdbstub.c | 10 +++++++++-
 lib/Kconfig.kgdb       |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 6198d2eb49c4..5bec444fc6d3 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -34,13 +34,14 @@
 #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*3, BUFMAX)];
 static int			gdbstub_use_prev_in_buf;
 static int			gdbstub_prev_in_buf_pos;
 
@@ -768,6 +769,13 @@ static void gdb_cmd_query(struct kgdb_state *ks)
 		*(--ptr) = '\0';
 		break;
 
+	case 'l':
+		if (memcmp(remcom_in_buffer + 1, "linux.vmcoreinfo", 16) == 0) {
+			remcom_out_buffer[0] = 'Q';
+			kgdb_mem2ebin(vmcoreinfo_data, remcom_out_buffer + 1, vmcoreinfo_size);
+		}
+		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
 	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

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

[Doug Anderson]

Hi,

On Wed, Dec 11, 2024 at 7:40 AM Amal Raj T <tjarlama@gmail.com> wrote:
>
> 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.

Can you add more documentation about `linux.vmcoreinfo`? Is there a
GDB patch that goes along with this that does something with it? I
assume that GDB patch would need the same magic escaping sequence
you've come up with?

How does one end up having a VM core for kgdb to serve up? Does it
automatically get generated before we enter kdb due to a crash now
that we select VMCORE_INFO, or is there some other mechanism?


> Maximum size of output buffer is set to 3X the maximum
> size of VMCOREINFO_BYTES (kgdb_mem2ebin() requires 1x
> for the temporary copy plus up to 2x for the escaped
> data).

Can you explain the 3x more? Specifically, it looks like the temporary
copy doesn't take up any extra space, unless I read your code wrong
(always possible).

Let's assume mem is '}}' and count is 2.

When the function starts you end up copying mem to the end of the
buffer. Thus, buffer will be '??}}'

The first time through the loop, you'll end filling in the escaped
first character and the buffer will be '}]}}'

The second time through the loop you'll have '}]}]'.

...so I think you need 2x the escaped data (or 2x +1 unless you remove
the '\0' termination from your earlier patch).

Oh, I guess you actually also need 1 extra byte for the 'Q' in your response?


> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  kernel/debug/gdbstub.c | 10 +++++++++-
>  lib/Kconfig.kgdb       |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index 6198d2eb49c4..5bec444fc6d3 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -34,13 +34,14 @@
>  #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*3, BUFMAX)];
>  static int                     gdbstub_use_prev_in_buf;
>  static int                     gdbstub_prev_in_buf_pos;
>
> @@ -768,6 +769,13 @@ static void gdb_cmd_query(struct kgdb_state *ks)
>                 *(--ptr) = '\0';
>                 break;
>
> +       case 'l':
> +               if (memcmp(remcom_in_buffer + 1, "linux.vmcoreinfo", 16) == 0) {

Is there termination in the `remcom_in_buffer`? If so, I'd rather see
a string comparison function used. Otherwise
`linux.vmcoreinfoWithExtraStuffAtTheEnd` will also match.

[PATCH v3 0/3] Add a new command in kgdb for vmcoreinfo

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

This patch set introduces a new custom query packet,
`qlinux.vmcoreinfo`, to kgdb. This packet retrieves kernel's
vmcoreinfo text, which contains crucial debugging information
such as the KASLR offset, kernel release, `phys_base` etc.

This implementation includes a new binary-encoder (`mem2ebin`)
instead of the existing hex-encoder (`mem2hex`) to reduce the
amount of data sent over the wire.

These changes also move the LF -> CRLF replacement logic from
serial drivers to KDB, since KDB is the only user of this
logic, and the existing replacement results in incorrect
checksums generated by KGDB.

Link:
- https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet

v3:
- Added relevant links for the new query
- Updated `tmp+=1` to `tmp++`
- Added few examples and additional documentation for encoding function
Amal Raj T (3):
  kgdb: Add kgdb_mem2ebin function for converting memory to binary
    format
  serial: Move LF -> CRLF replacement from serial console to kdb
  kgdb: Add command linux.vmcoreinfo to kgdb

 drivers/tty/serial/serial_core.c |   2 -
 include/linux/kgdb.h             |   1 +
 kernel/debug/gdbstub.c           | 131 ++++++++++++++++++++-----------
 kernel/debug/kdb/kdb_io.c        |   2 +
 lib/Kconfig.kgdb                 |   1 +
 5 files changed, 91 insertions(+), 46 deletions(-)

-- 
2.43.5

[PATCH v3 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

Add a new function kgdb_mem2ebin that converts memory to binary
format, escaping special characters ('$', '#', '*', and '}').
These ASCII characters have the following meaning in GDB binary encoding
- `$`: Seven repeats in run-length encoding
- `#`: Six repeats
- `*`: Three repeats
- `}`: Used as escape character
kgdb_mem2ebin function ensures that memory data
is properly formatted and escaped before being
sent over the wire. Additionally, this function
reduces the amount of data exchanged between
debugger compared to hex.

Link: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Overview.html#Binary-Data

Signed-off-by: Amal Raj T <amalrajt@meta.com>
---
 include/linux/kgdb.h   |   1 +
 kernel/debug/gdbstub.c | 119 ++++++++++++++++++++++++++---------------
 2 files changed, 77 insertions(+), 43 deletions(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 76e891ee9e37..fa3cf38a14de 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
 
 extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
 extern char *kgdb_mem2hex(char *mem, char *buf, int count);
+extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
 extern int kgdb_hex2mem(char *buf, char *mem, int count);
 
 extern int kgdb_isremovedbreak(unsigned long addr);
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index f625172d4b67..f88e21d5502a 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -39,15 +39,14 @@
 #define KGDB_MAX_THREAD_QUERY 17
 
 /* Our I/O buffers. */
-static char			remcom_in_buffer[BUFMAX];
-static char			remcom_out_buffer[BUFMAX];
-static int			gdbstub_use_prev_in_buf;
-static int			gdbstub_prev_in_buf_pos;
+static char remcom_in_buffer[BUFMAX];
+static char remcom_out_buffer[BUFMAX];
+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)];
+static unsigned long gdb_regs[(NUMREGBYTES + sizeof(unsigned long) - 1) /
+			      sizeof(unsigned long)];
 
 /*
  * GDB remote protocol parser:
@@ -257,6 +256,49 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
 	return buf;
 }
 
+/**
+ * Convert memory to binary format for GDB remote protocol transmission,
+ * escaping special characters ($, #, *, and }) using the `}` character as an escape.
+ *
+ * The `$` (seven repeats), `#`(six repeats), `*`(three run-length), and `}`
+ * characters are considered special because they have
+ * specific meanings in the GDB remote protocol. To avoid conflicts, these
+ * characters are escaped by prefixing them with the `}` character and then XORing
+ * the original character with 0x20.
+ *
+ * Examples:
+ *   - `0x7d` (ASCII '}') is transmitted as `0x7d 0x5d`
+ *   - `0x23` (ASCII '#') is transmitted as `0x7d 0x43`
+ */
+char *kgdb_mem2ebin(char *mem, char *buf, int count)
+{
+	char *tmp;
+	int err;
+
+	/* We use the upper half of buf as an intermediate buffer
+	 *	for the raw memory copy.
+	 */
+	tmp = buf + count;
+
+	err = copy_from_kernel_nofault(tmp, mem, count);
+	if (err)
+		return NULL;
+	while (count > 0) {
+		unsigned char c = *tmp;
+
+		if (c == '}' || c == '#' || c == '*' || c == '#') {
+			*buf++ = '}';
+			*buf++ = c ^ 0x20;
+		} else {
+			*buf++ = c;
+		}
+		count -= 1;
+		tmp += 1;
+	}
+
+	return buf;
+}
+
 /*
  * Convert the hex array pointed to by buf into binary to be placed in
  * mem.  Return a pointer to the character AFTER the last byte
@@ -400,7 +442,7 @@ static void error_packet(char *pkt, int error)
  * remapped to negative TIDs.
  */
 
-#define BUF_THREAD_ID_SIZE	8
+#define BUF_THREAD_ID_SIZE 8
 
 static char *pack_threadid(char *pkt, unsigned char *id)
 {
@@ -454,7 +496,6 @@ static struct task_struct *getthread(struct pt_regs *regs, int tid)
 	return find_task_by_pid_ns(tid, &init_pid_ns);
 }
 
-
 /*
  * Remap normal tasks to their real PID,
  * CPU shadow threads are mapped to -CPU - 2
@@ -560,7 +601,7 @@ static void gdb_cmd_memread(struct kgdb_state *ks)
 	char *err;
 
 	if (kgdb_hex2long(&ptr, &addr) > 0 && *ptr++ == ',' &&
-					kgdb_hex2long(&ptr, &length) > 0) {
+	    kgdb_hex2long(&ptr, &length) > 0) {
 		err = kgdb_mem2hex((char *)addr, remcom_out_buffer, length);
 		if (!err)
 			error_packet(remcom_out_buffer, -EINVAL);
@@ -615,8 +656,7 @@ static void gdb_cmd_reg_set(struct kgdb_state *ks)
 	int i = 0;
 
 	kgdb_hex2long(&ptr, &regnum);
-	if (*ptr++ != '=' ||
-	    !(!kgdb_usethread || kgdb_usethread == current) ||
+	if (*ptr++ != '=' || !(!kgdb_usethread || kgdb_usethread == current) ||
 	    !dbg_get_reg(regnum, gdb_regs, ks->linux_regs)) {
 		error_packet(remcom_out_buffer, -EINVAL);
 		return;
@@ -756,14 +796,14 @@ static void gdb_cmd_query(struct kgdb_state *ks)
 			break;
 		}
 		if ((int)ks->threadid > 0) {
-			kgdb_mem2hex(getthread(ks->linux_regs,
-					ks->threadid)->comm,
-					remcom_out_buffer, 16);
+			kgdb_mem2hex(
+				getthread(ks->linux_regs, ks->threadid)->comm,
+				remcom_out_buffer, 16);
 		} else {
 			static char tmpstr[23 + BUF_THREAD_ID_SIZE];
 
 			sprintf(tmpstr, "shadowCPU%d",
-					(int)(-ks->threadid - 2));
+				(int)(-ks->threadid - 2));
 			kgdb_mem2hex(tmpstr, remcom_out_buffer, strlen(tmpstr));
 		}
 		break;
@@ -776,8 +816,8 @@ static void gdb_cmd_query(struct kgdb_state *ks)
 				strcpy(remcom_out_buffer, "E01");
 				break;
 			}
-			kgdb_hex2mem(remcom_in_buffer + 6,
-				     remcom_out_buffer, len);
+			kgdb_hex2mem(remcom_in_buffer + 6, remcom_out_buffer,
+				     len);
 			len = len / 2;
 			remcom_out_buffer[len++] = 0;
 
@@ -895,8 +935,7 @@ static void gdb_cmd_break(struct kgdb_state *ks)
 		error_packet(remcom_out_buffer, -EINVAL);
 		return;
 	}
-	if (*(ptr++) != ',' ||
-		!kgdb_hex2long(&ptr, &length)) {
+	if (*(ptr++) != ',' || !kgdb_hex2long(&ptr, &length)) {
 		error_packet(remcom_out_buffer, -EINVAL);
 		return;
 	}
@@ -906,11 +945,11 @@ static void gdb_cmd_break(struct kgdb_state *ks)
 	else if (remcom_in_buffer[0] == 'z' && *bpt_type == '0')
 		error = dbg_remove_sw_break(addr);
 	else if (remcom_in_buffer[0] == 'Z')
-		error = arch_kgdb_ops.set_hw_breakpoint(addr,
-			(int)length, *bpt_type - '0');
+		error = arch_kgdb_ops.set_hw_breakpoint(addr, (int)length,
+							*bpt_type - '0');
 	else if (remcom_in_buffer[0] == 'z')
-		error = arch_kgdb_ops.remove_hw_breakpoint(addr,
-			(int) length, *bpt_type - '0');
+		error = arch_kgdb_ops.remove_hw_breakpoint(addr, (int)length,
+							   *bpt_type - '0');
 
 	if (error == 0)
 		strcpy(remcom_out_buffer, "OK");
@@ -925,12 +964,10 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
 	 * C15 == detach kgdb, pass exception
 	 */
 	if (remcom_in_buffer[1] == '0' && remcom_in_buffer[2] == '9') {
-
 		ks->pass_exception = 1;
 		remcom_in_buffer[0] = 'c';
 
 	} else if (remcom_in_buffer[1] == '1' && remcom_in_buffer[2] == '5') {
-
 		ks->pass_exception = 1;
 		remcom_in_buffer[0] = 'D';
 		dbg_remove_all_break();
@@ -938,9 +975,11 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
 		return 1;
 
 	} else {
-		gdbstub_msg_write("KGDB only knows signal 9 (pass)"
+		gdbstub_msg_write(
+			"KGDB only knows signal 9 (pass)"
 			" and 15 (pass and disconnect)\n"
-			"Executing a continue without signal passing\n", 0);
+			"Executing a continue without signal passing\n",
+			0);
 		remcom_in_buffer[0] = 'c';
 	}
 
@@ -1050,7 +1089,7 @@ int gdb_serial_stub(struct kgdb_state *ks)
 				goto default_handle;
 			if (tmp == 0)
 				break;
-			fallthrough;	/* on tmp < 0 */
+			fallthrough; /* on tmp < 0 */
 		case 'c': /* Continue packet */
 		case 's': /* Single step packet */
 			if (kgdb_contthread && kgdb_contthread != current) {
@@ -1058,15 +1097,13 @@ int gdb_serial_stub(struct kgdb_state *ks)
 				error_packet(remcom_out_buffer, -EINVAL);
 				break;
 			}
-			fallthrough;	/* to default processing */
+			fallthrough; /* to default processing */
 		default:
 default_handle:
-			error = kgdb_arch_handle_exception(ks->ex_vector,
-						ks->signo,
-						ks->err_code,
-						remcom_in_buffer,
-						remcom_out_buffer,
-						ks->linux_regs);
+			error = kgdb_arch_handle_exception(
+				ks->ex_vector, ks->signo, ks->err_code,
+				remcom_in_buffer, remcom_out_buffer,
+				ks->linux_regs);
 			/*
 			 * Leave cmd processing on error, detach,
 			 * kill, continue, or single step.
@@ -1076,7 +1113,6 @@ int gdb_serial_stub(struct kgdb_state *ks)
 				error = 0;
 				goto kgdb_exit;
 			}
-
 		}
 
 		/* reply to the request */
@@ -1095,12 +1131,9 @@ int gdbstub_state(struct kgdb_state *ks, char *cmd)
 
 	switch (cmd[0]) {
 	case 'e':
-		error = kgdb_arch_handle_exception(ks->ex_vector,
-						   ks->signo,
-						   ks->err_code,
-						   remcom_in_buffer,
-						   remcom_out_buffer,
-						   ks->linux_regs);
+		error = kgdb_arch_handle_exception(
+			ks->ex_vector, ks->signo, ks->err_code,
+			remcom_in_buffer, remcom_out_buffer, ks->linux_regs);
 		return error;
 	case 's':
 	case 'c':
-- 
2.43.5

Re: [PATCH v3 1/3] kgdb: Add kgdb_mem2ebin function for converting memory to binary format

[Stephen Brennan]

Hi Amal,

Amal Raj T <tjarlama@gmail.com> writes:
> From: Amal Raj T <amalrajt@meta.com>
>
> Add a new function kgdb_mem2ebin that converts memory to binary
> format, escaping special characters ('$', '#', '*', and '}').
> These ASCII characters have the following meaning in GDB binary encoding
> - `$`: Seven repeats in run-length encoding
> - `#`: Six repeats
> - `*`: Three repeats

These aren't the justification for escaping. From the GDB doc linked
below:

- '$' is used to introduce a packet
- '#' is used to end a packet

These cannot appear in any of the packet contents, otherwise they would
confuse the code which determines the packet framing. So they must be
escaped.

In fact, in run-length encoding, these characters are banned -- so they
don't represent seven or six repeats, they simply cannot be used, for
the same reason.

- '*' is used to introduce a run-length encoding

Note that the language of the document indicates that only the "stub"
must escape the '*', because only the stub can use run-length encoding.
Presumably the debugger side of the connection need not escape it,
because the protocol is designed to allow the stub to be implemented
more simply, without needing how to interpret run-length encodings.

I don't think you need to include that level of detail, but the short
description of each character and the reasoning would be good. And put
the same corrected description in the comment below.

> - `}`: Used as escape character
> kgdb_mem2ebin function ensures that memory data
> is properly formatted and escaped before being
> sent over the wire. Additionally, this function
> reduces the amount of data exchanged between
> debugger compared to hex.
>
> Link: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Overview.html#Binary-Data
>
> Signed-off-by: Amal Raj T <amalrajt@meta.com>
> ---
>  include/linux/kgdb.h   |   1 +
>  kernel/debug/gdbstub.c | 119 ++++++++++++++++++++++++++---------------
>  2 files changed, 77 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 76e891ee9e37..fa3cf38a14de 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -322,6 +322,7 @@ extern struct kgdb_io *dbg_io_ops;
>  
>  extern int kgdb_hex2long(char **ptr, unsigned long *long_val);
>  extern char *kgdb_mem2hex(char *mem, char *buf, int count);
> +extern char *kgdb_mem2ebin(char *mem, char *buf, int count);
>  extern int kgdb_hex2mem(char *buf, char *mem, int count);
>  
>  extern int kgdb_isremovedbreak(unsigned long addr);
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index f625172d4b67..f88e21d5502a 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -39,15 +39,14 @@
>  #define KGDB_MAX_THREAD_QUERY 17
>  
>  /* Our I/O buffers. */
> -static char			remcom_in_buffer[BUFMAX];
> -static char			remcom_out_buffer[BUFMAX];
> -static int			gdbstub_use_prev_in_buf;
> -static int			gdbstub_prev_in_buf_pos;
> +static char remcom_in_buffer[BUFMAX];
> +static char remcom_out_buffer[BUFMAX];
> +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)];
> +static unsigned long gdb_regs[(NUMREGBYTES + sizeof(unsigned long) - 1) /
> +			      sizeof(unsigned long)];

It looks like the majority of the code changes in this patch are
formatting, maybe due to an automatic formatter?

Generally it's bad practice to include those sorts of changes since
they're unrelated to the subject of this patch.

>  
>  /*
>   * GDB remote protocol parser:
> @@ -257,6 +256,49 @@ char *kgdb_mem2hex(char *mem, char *buf, int count)
>  	return buf;
>  }
>  
> +/**
> + * Convert memory to binary format for GDB remote protocol transmission,
> + * escaping special characters ($, #, *, and }) using the `}` character as an escape.
> + *
> + * The `$` (seven repeats), `#`(six repeats), `*`(three run-length), and `}`
> + * characters are considered special because they have
> + * specific meanings in the GDB remote protocol. To avoid conflicts, these
> + * characters are escaped by prefixing them with the `}` character and then XORing
> + * the original character with 0x20.
> + *
> + * Examples:
> + *   - `0x7d` (ASCII '}') is transmitted as `0x7d 0x5d`
> + *   - `0x23` (ASCII '#') is transmitted as `0x7d 0x43`
> + */
> +char *kgdb_mem2ebin(char *mem, char *buf, int count)
> +{
> +	char *tmp;
> +	int err;
> +
> +	/* We use the upper half of buf as an intermediate buffer
> +	 *	for the raw memory copy.
> +	 */
> +	tmp = buf + count;

It's a fun mental experiment to confirm that, even if every character in
the source buffer required escaping, this function would still work :)

> +	err = copy_from_kernel_nofault(tmp, mem, count);
> +	if (err)
> +		return NULL;
> +	while (count > 0) {
> +		unsigned char c = *tmp;
> +
> +		if (c == '}' || c == '#' || c == '*' || c == '#') {
> +			*buf++ = '}';
> +			*buf++ = c ^ 0x20;
> +		} else {
> +			*buf++ = c;
> +		}
> +		count -= 1;
> +		tmp += 1;
> +	}

Give that the caller can't know how many bytes the escaped data will
consume, how does this function return that information? Is the idea
that "count" includes the nul byte, and so the final iteration of this
loop will nul-terminate the buffer?

Thanks,
Stephen

> +	return buf;
> +}
> +
>  /*
>   * Convert the hex array pointed to by buf into binary to be placed in
>   * mem.  Return a pointer to the character AFTER the last byte
> @@ -400,7 +442,7 @@ static void error_packet(char *pkt, int error)
>   * remapped to negative TIDs.
>   */
>  
> -#define BUF_THREAD_ID_SIZE	8
> +#define BUF_THREAD_ID_SIZE 8
>  
>  static char *pack_threadid(char *pkt, unsigned char *id)
>  {
> @@ -454,7 +496,6 @@ static struct task_struct *getthread(struct pt_regs *regs, int tid)
>  	return find_task_by_pid_ns(tid, &init_pid_ns);
>  }
>  
> -
>  /*
>   * Remap normal tasks to their real PID,
>   * CPU shadow threads are mapped to -CPU - 2
> @@ -560,7 +601,7 @@ static void gdb_cmd_memread(struct kgdb_state *ks)
>  	char *err;
>  
>  	if (kgdb_hex2long(&ptr, &addr) > 0 && *ptr++ == ',' &&
> -					kgdb_hex2long(&ptr, &length) > 0) {
> +	    kgdb_hex2long(&ptr, &length) > 0) {
>  		err = kgdb_mem2hex((char *)addr, remcom_out_buffer, length);
>  		if (!err)
>  			error_packet(remcom_out_buffer, -EINVAL);
> @@ -615,8 +656,7 @@ static void gdb_cmd_reg_set(struct kgdb_state *ks)
>  	int i = 0;
>  
>  	kgdb_hex2long(&ptr, &regnum);
> -	if (*ptr++ != '=' ||
> -	    !(!kgdb_usethread || kgdb_usethread == current) ||
> +	if (*ptr++ != '=' || !(!kgdb_usethread || kgdb_usethread == current) ||
>  	    !dbg_get_reg(regnum, gdb_regs, ks->linux_regs)) {
>  		error_packet(remcom_out_buffer, -EINVAL);
>  		return;
> @@ -756,14 +796,14 @@ static void gdb_cmd_query(struct kgdb_state *ks)
>  			break;
>  		}
>  		if ((int)ks->threadid > 0) {
> -			kgdb_mem2hex(getthread(ks->linux_regs,
> -					ks->threadid)->comm,
> -					remcom_out_buffer, 16);
> +			kgdb_mem2hex(
> +				getthread(ks->linux_regs, ks->threadid)->comm,
> +				remcom_out_buffer, 16);
>  		} else {
>  			static char tmpstr[23 + BUF_THREAD_ID_SIZE];
>  
>  			sprintf(tmpstr, "shadowCPU%d",
> -					(int)(-ks->threadid - 2));
> +				(int)(-ks->threadid - 2));
>  			kgdb_mem2hex(tmpstr, remcom_out_buffer, strlen(tmpstr));
>  		}
>  		break;
> @@ -776,8 +816,8 @@ static void gdb_cmd_query(struct kgdb_state *ks)
>  				strcpy(remcom_out_buffer, "E01");
>  				break;
>  			}
> -			kgdb_hex2mem(remcom_in_buffer + 6,
> -				     remcom_out_buffer, len);
> +			kgdb_hex2mem(remcom_in_buffer + 6, remcom_out_buffer,
> +				     len);
>  			len = len / 2;
>  			remcom_out_buffer[len++] = 0;
>  
> @@ -895,8 +935,7 @@ static void gdb_cmd_break(struct kgdb_state *ks)
>  		error_packet(remcom_out_buffer, -EINVAL);
>  		return;
>  	}
> -	if (*(ptr++) != ',' ||
> -		!kgdb_hex2long(&ptr, &length)) {
> +	if (*(ptr++) != ',' || !kgdb_hex2long(&ptr, &length)) {
>  		error_packet(remcom_out_buffer, -EINVAL);
>  		return;
>  	}
> @@ -906,11 +945,11 @@ static void gdb_cmd_break(struct kgdb_state *ks)
>  	else if (remcom_in_buffer[0] == 'z' && *bpt_type == '0')
>  		error = dbg_remove_sw_break(addr);
>  	else if (remcom_in_buffer[0] == 'Z')
> -		error = arch_kgdb_ops.set_hw_breakpoint(addr,
> -			(int)length, *bpt_type - '0');
> +		error = arch_kgdb_ops.set_hw_breakpoint(addr, (int)length,
> +							*bpt_type - '0');
>  	else if (remcom_in_buffer[0] == 'z')
> -		error = arch_kgdb_ops.remove_hw_breakpoint(addr,
> -			(int) length, *bpt_type - '0');
> +		error = arch_kgdb_ops.remove_hw_breakpoint(addr, (int)length,
> +							   *bpt_type - '0');
>  
>  	if (error == 0)
>  		strcpy(remcom_out_buffer, "OK");
> @@ -925,12 +964,10 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
>  	 * C15 == detach kgdb, pass exception
>  	 */
>  	if (remcom_in_buffer[1] == '0' && remcom_in_buffer[2] == '9') {
> -
>  		ks->pass_exception = 1;
>  		remcom_in_buffer[0] = 'c';
>  
>  	} else if (remcom_in_buffer[1] == '1' && remcom_in_buffer[2] == '5') {
> -
>  		ks->pass_exception = 1;
>  		remcom_in_buffer[0] = 'D';
>  		dbg_remove_all_break();
> @@ -938,9 +975,11 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
>  		return 1;
>  
>  	} else {
> -		gdbstub_msg_write("KGDB only knows signal 9 (pass)"
> +		gdbstub_msg_write(
> +			"KGDB only knows signal 9 (pass)"
>  			" and 15 (pass and disconnect)\n"
> -			"Executing a continue without signal passing\n", 0);
> +			"Executing a continue without signal passing\n",
> +			0);
>  		remcom_in_buffer[0] = 'c';
>  	}
>  
> @@ -1050,7 +1089,7 @@ int gdb_serial_stub(struct kgdb_state *ks)
>  				goto default_handle;
>  			if (tmp == 0)
>  				break;
> -			fallthrough;	/* on tmp < 0 */
> +			fallthrough; /* on tmp < 0 */
>  		case 'c': /* Continue packet */
>  		case 's': /* Single step packet */
>  			if (kgdb_contthread && kgdb_contthread != current) {
> @@ -1058,15 +1097,13 @@ int gdb_serial_stub(struct kgdb_state *ks)
>  				error_packet(remcom_out_buffer, -EINVAL);
>  				break;
>  			}
> -			fallthrough;	/* to default processing */
> +			fallthrough; /* to default processing */
>  		default:
>  default_handle:
> -			error = kgdb_arch_handle_exception(ks->ex_vector,
> -						ks->signo,
> -						ks->err_code,
> -						remcom_in_buffer,
> -						remcom_out_buffer,
> -						ks->linux_regs);
> +			error = kgdb_arch_handle_exception(
> +				ks->ex_vector, ks->signo, ks->err_code,
> +				remcom_in_buffer, remcom_out_buffer,
> +				ks->linux_regs);
>  			/*
>  			 * Leave cmd processing on error, detach,
>  			 * kill, continue, or single step.
> @@ -1076,7 +1113,6 @@ int gdb_serial_stub(struct kgdb_state *ks)
>  				error = 0;
>  				goto kgdb_exit;
>  			}
> -
>  		}
>  
>  		/* reply to the request */
> @@ -1095,12 +1131,9 @@ int gdbstub_state(struct kgdb_state *ks, char *cmd)
>  
>  	switch (cmd[0]) {
>  	case 'e':
> -		error = kgdb_arch_handle_exception(ks->ex_vector,
> -						   ks->signo,
> -						   ks->err_code,
> -						   remcom_in_buffer,
> -						   remcom_out_buffer,
> -						   ks->linux_regs);
> +		error = kgdb_arch_handle_exception(
> +			ks->ex_vector, ks->signo, ks->err_code,
> +			remcom_in_buffer, remcom_out_buffer, ks->linux_regs);
>  		return error;
>  	case 's':
>  	case 'c':
> -- 
> 2.43.5

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

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

The current implementation of `poll_put_char` in the serial console driver
performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
is the only user of `poll_put_char`, this patch moves the LF -> CRLF
replacement logic to kdb.

Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
---
 drivers/tty/serial/serial_core.c | 2 --
 kernel/debug/kdb/kdb_io.c        | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74fa02b23772..8e702f3deffb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2738,8 +2738,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	if (!port)
 		return;
 
-	if (ch == '\n')
-		port->ops->poll_put_char(port, '\r');
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 6a77f1c779c4..43a7c8ad741a 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	len = msg_len;
 
 	while (len--) {
+		if (*cp == '\n')
+			dbg_io_ops->write_char('\r');
 		dbg_io_ops->write_char(*cp);
 		cp++;
 	}
-- 
2.43.5

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

[Stephen Brennan]

Hi Amal,

There are two "PATCH v3 2/3" messages. This one's contents do not match
its subject line, so I assume that the correct one is "[PATCH v3 2/3]
serial: Move LF -> CRLF replacement from serial console to kdb".

Thanks,
Stephen

Amal Raj T <tjarlama@gmail.com> writes:
> From: Amal Raj T <amalrajt@meta.com>
>
> The current implementation of `poll_put_char` in the serial console driver
> performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> replacement logic to kdb.
>
> Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
> ---
>  drivers/tty/serial/serial_core.c | 2 --
>  kernel/debug/kdb/kdb_io.c        | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 74fa02b23772..8e702f3deffb 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2738,8 +2738,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
>  	if (!port)
>  		return;
>  
> -	if (ch == '\n')
> -		port->ops->poll_put_char(port, '\r');
>  	port->ops->poll_put_char(port, ch);
>  	uart_port_deref(port);
>  }
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 6a77f1c779c4..43a7c8ad741a 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	len = msg_len;
>  
>  	while (len--) {
> +		if (*cp == '\n')
> +			dbg_io_ops->write_char('\r');
>  		dbg_io_ops->write_char(*cp);
>  		cp++;
>  	}
> -- 
> 2.43.5

[PATCH v3 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Amal Raj T]

From: Amal Raj T <amalrajt@meta.com>

The current implementation of `poll_put_char` in the serial console driver
performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
is the only user of `poll_put_char`, this patch moves the LF -> CRLF
replacement logic to kdb.

Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
---
 drivers/tty/serial/serial_core.c | 2 --
 kernel/debug/kdb/kdb_io.c        | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74fa02b23772..8e702f3deffb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2738,8 +2738,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	if (!port)
 		return;
 
-	if (ch == '\n')
-		port->ops->poll_put_char(port, '\r');
 	port->ops->poll_put_char(port, ch);
 	uart_port_deref(port);
 }
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 6a77f1c779c4..43a7c8ad741a 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	len = msg_len;
 
 	while (len--) {
+		if (*cp == '\n')
+			dbg_io_ops->write_char('\r');
 		dbg_io_ops->write_char(*cp);
 		cp++;
 	}
-- 
2.43.5

Re: [PATCH v3 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Stephen Brennan]

Amal Raj T <tjarlama@gmail.com> writes:

> From: Amal Raj T <amalrajt@meta.com>
>
> The current implementation of `poll_put_char` in the serial console driver
> performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> replacement logic to kdb.
>
> Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/

Reviewed-by: Stephen Brennan <stephen.s.brennan@oracle.com>

> ---
>  drivers/tty/serial/serial_core.c | 2 --
>  kernel/debug/kdb/kdb_io.c        | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 74fa02b23772..8e702f3deffb 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2738,8 +2738,6 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
>  	if (!port)
>  		return;
>  
> -	if (ch == '\n')
> -		port->ops->poll_put_char(port, '\r');
>  	port->ops->poll_put_char(port, ch);
>  	uart_port_deref(port);
>  }
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 6a77f1c779c4..43a7c8ad741a 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -572,6 +572,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	len = msg_len;
>  
>  	while (len--) {
> +		if (*cp == '\n')
> +			dbg_io_ops->write_char('\r');
>  		dbg_io_ops->write_char(*cp);
>  		cp++;
>  	}
> -- 
> 2.43.5

Re: [PATCH v3 2/3] serial: Move LF -> CRLF replacement from serial console to kdb

[Doug Anderson]

Hi,

On Mon, Jan 13, 2025 at 9:29 AM Amal Raj T <tjarlama@gmail.com> wrote:
>
> From: Amal Raj T <amalrajt@meta.com>
>
> The current implementation of `poll_put_char` in the serial console driver
> performs LF -> CRLF replacement, which can corrupt binary data. Since kdb
> is the only user of `poll_put_char`, this patch moves the LF -> CRLF
> replacement logic to kdb.
>
> Link: https://lore.kernel.org/linux-debuggers/Zy093jVKPs9gSVx2@telecaster/
> ---
>  drivers/tty/serial/serial_core.c | 2 --
>  kernel/debug/kdb/kdb_io.c        | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

You seem to have dropped your Signed-off-by? It was there on V2 [1].
...and speaking of V2, I provided my Reviewed-by there. You should
have carried it forward since there are no differences in the code
between V2 and V3. I can give it again, but it would be nice not to
need to repeat when you send the V4.

Once you've added back your own Signed-off-by, you can also add:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

...also speaking of V2, I did ask if you could maybe add something to
the commit message pointing at the previous discussion. If you really
have a reason not to do this I won't insist, but it would be nice to
include it...

[1] https://lore.kernel.org/all/20241211153955.33518-3-tjarlama@gmail.com/

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

[Amal Raj T]

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)];
 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++;
 	}
 
 	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);
+		}
+		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
 	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

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

[Stephen Brennan]

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.

>  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

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

[Stephen Brennan]

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