kgdb replacing newline with CRLF in custom query response

kgdb replacing newline with CRLF in custom query response

[Omar Sandoval]

Hi everyone,

Amal is working on adding a custom query packet to kgdb for getting the
kernel's vmcoreinfo. The rationale and details are available here:
https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet

vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
response and doubling the time it takes to transmit over a slow serial
connection. Instead, we were hoping to use the escaped binary format,
which escapes the characters #$}* and leaves other bytes untouched.

We ran into a problem, though: vmcoreinfo contains newline characters,
which the serial core replaces with CRLF; see commit c7d44a02ac60
("serial_core: Commonalize crlf when working w/ a non open console
port"). This effectively corrupts the data and causes a checksum
mismatch.

We'd love some input on how to work around this, especially from the
kgdb maintainers. Here are a few options, in descending order of my
preference:

1. Disable the LF -> CRLF replacement while sending binary data.
2. Escape the newlines using some other custom scheme.
3. Give up and hex-encode the response.

Any thoughts?

Thanks,
Omar

Re: kgdb replacing newline with CRLF in custom query response

[Doug Anderson]

Hi,

On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> Hi everyone,
>
> Amal is working on adding a custom query packet to kgdb for getting the
> kernel's vmcoreinfo. The rationale and details are available here:
> https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
>
> vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> response and doubling the time it takes to transmit over a slow serial
> connection. Instead, we were hoping to use the escaped binary format,
> which escapes the characters #$}* and leaves other bytes untouched.
>
> We ran into a problem, though: vmcoreinfo contains newline characters,
> which the serial core replaces with CRLF; see commit c7d44a02ac60
> ("serial_core: Commonalize crlf when working w/ a non open console
> port").

FWIW, the problem predates that commit, but that commit at least moved
it to be someplace common. Before that some serial drivers were
hardcoding it... ;-)


> This effectively corrupts the data and causes a checksum
> mismatch.
>
> We'd love some input on how to work around this, especially from the
> kgdb maintainers. Here are a few options, in descending order of my
> preference:
>
> 1. Disable the LF -> CRLF replacement while sending binary data.
> 2. Escape the newlines using some other custom scheme.
> 3. Give up and hex-encode the response.

I haven't tried prototyping it, but what about moving the LR -> CRLF
code to kdb_msg_write(). It would be really easy to do this in the
case where we're doing "dbg_io_ops->write_char()" since we're already
processing character at a time. It would be harder to do this when
also sending the output to the various console, but may not _too_
hard? You could loop searching for "\n" and send all the characters
before the "\n", then send a "\r", then send the "\n" and all the
characters up to the next "\n".

If you did this then you'd lose the "\n" to "\r\n" combination in the
gdb stub, but _probably_ that doesn't matter?

-Doug

Re: kgdb replacing newline with CRLF in custom query response

[Omar Sandoval]

On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > Hi everyone,
> >
> > Amal is working on adding a custom query packet to kgdb for getting the
> > kernel's vmcoreinfo. The rationale and details are available here:
> > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
> >
> > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> > response and doubling the time it takes to transmit over a slow serial
> > connection. Instead, we were hoping to use the escaped binary format,
> > which escapes the characters #$}* and leaves other bytes untouched.
> >
> > We ran into a problem, though: vmcoreinfo contains newline characters,
> > which the serial core replaces with CRLF; see commit c7d44a02ac60
> > ("serial_core: Commonalize crlf when working w/ a non open console
> > port").
> 
> FWIW, the problem predates that commit, but that commit at least moved
> it to be someplace common. Before that some serial drivers were
> hardcoding it... ;-)
> 
> 
> > This effectively corrupts the data and causes a checksum
> > mismatch.
> >
> > We'd love some input on how to work around this, especially from the
> > kgdb maintainers. Here are a few options, in descending order of my
> > preference:
> >
> > 1. Disable the LF -> CRLF replacement while sending binary data.
> > 2. Escape the newlines using some other custom scheme.
> > 3. Give up and hex-encode the response.
> 
> I haven't tried prototyping it, but what about moving the LR -> CRLF
> code to kdb_msg_write(). It would be really easy to do this in the
> case where we're doing "dbg_io_ops->write_char()" since we're already
> processing character at a time. It would be harder to do this when
> also sending the output to the various console, but may not _too_
> hard? You could loop searching for "\n" and send all the characters
> before the "\n", then send a "\r", then send the "\n" and all the
> characters up to the next "\n".
> 
> If you did this then you'd lose the "\n" to "\r\n" combination in the
> gdb stub, but _probably_ that doesn't matter?

That sounds reasonable. I was concerned whether this would affect
anything else using the ->poll_put_char() tty operation, but kgdb seems
to be the only user, does that sound right?

Thanks for the quick response!

Omar

Re: kgdb replacing newline with CRLF in custom query response

[Doug Anderson]

Hi,

On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
> > >
> > > Hi everyone,
> > >
> > > Amal is working on adding a custom query packet to kgdb for getting the
> > > kernel's vmcoreinfo. The rationale and details are available here:
> > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
> > >
> > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> > > response and doubling the time it takes to transmit over a slow serial
> > > connection. Instead, we were hoping to use the escaped binary format,
> > > which escapes the characters #$}* and leaves other bytes untouched.
> > >
> > > We ran into a problem, though: vmcoreinfo contains newline characters,
> > > which the serial core replaces with CRLF; see commit c7d44a02ac60
> > > ("serial_core: Commonalize crlf when working w/ a non open console
> > > port").
> >
> > FWIW, the problem predates that commit, but that commit at least moved
> > it to be someplace common. Before that some serial drivers were
> > hardcoding it... ;-)
> >
> >
> > > This effectively corrupts the data and causes a checksum
> > > mismatch.
> > >
> > > We'd love some input on how to work around this, especially from the
> > > kgdb maintainers. Here are a few options, in descending order of my
> > > preference:
> > >
> > > 1. Disable the LF -> CRLF replacement while sending binary data.
> > > 2. Escape the newlines using some other custom scheme.
> > > 3. Give up and hex-encode the response.
> >
> > I haven't tried prototyping it, but what about moving the LR -> CRLF
> > code to kdb_msg_write(). It would be really easy to do this in the
> > case where we're doing "dbg_io_ops->write_char()" since we're already
> > processing character at a time. It would be harder to do this when
> > also sending the output to the various console, but may not _too_
> > hard? You could loop searching for "\n" and send all the characters
> > before the "\n", then send a "\r", then send the "\n" and all the
> > characters up to the next "\n".
> >
> > If you did this then you'd lose the "\n" to "\r\n" combination in the
> > gdb stub, but _probably_ that doesn't matter?
>
> That sounds reasonable. I was concerned whether this would affect
> anything else using the ->poll_put_char() tty operation, but kgdb seems
> to be the only user, does that sound right?

Right. As far as I can tell, kgdb is the only user of poll_put_char().

-Doug

Re: kgdb replacing newline with CRLF in custom query response

[Omar Sandoval]

On Fri, Nov 08, 2024 at 07:31:19AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > >
> > > > Hi everyone,
> > > >
> > > > Amal is working on adding a custom query packet to kgdb for getting the
> > > > kernel's vmcoreinfo. The rationale and details are available here:
> > > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
> > > >
> > > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> > > > response and doubling the time it takes to transmit over a slow serial
> > > > connection. Instead, we were hoping to use the escaped binary format,
> > > > which escapes the characters #$}* and leaves other bytes untouched.
> > > >
> > > > We ran into a problem, though: vmcoreinfo contains newline characters,
> > > > which the serial core replaces with CRLF; see commit c7d44a02ac60
> > > > ("serial_core: Commonalize crlf when working w/ a non open console
> > > > port").
> > >
> > > FWIW, the problem predates that commit, but that commit at least moved
> > > it to be someplace common. Before that some serial drivers were
> > > hardcoding it... ;-)
> > >
> > >
> > > > This effectively corrupts the data and causes a checksum
> > > > mismatch.
> > > >
> > > > We'd love some input on how to work around this, especially from the
> > > > kgdb maintainers. Here are a few options, in descending order of my
> > > > preference:
> > > >
> > > > 1. Disable the LF -> CRLF replacement while sending binary data.
> > > > 2. Escape the newlines using some other custom scheme.
> > > > 3. Give up and hex-encode the response.
> > >
> > > I haven't tried prototyping it, but what about moving the LR -> CRLF
> > > code to kdb_msg_write(). It would be really easy to do this in the
> > > case where we're doing "dbg_io_ops->write_char()" since we're already
> > > processing character at a time. It would be harder to do this when
> > > also sending the output to the various console, but may not _too_
> > > hard? You could loop searching for "\n" and send all the characters
> > > before the "\n", then send a "\r", then send the "\n" and all the
> > > characters up to the next "\n".
> > >
> > > If you did this then you'd lose the "\n" to "\r\n" combination in the
> > > gdb stub, but _probably_ that doesn't matter?
> >
> > That sounds reasonable. I was concerned whether this would affect
> > anything else using the ->poll_put_char() tty operation, but kgdb seems
> > to be the only user, does that sound right?
> 
> Right. As far as I can tell, kgdb is the only user of poll_put_char().

Ah, one other concern, though: only uart_poll_put_char() does the CRLF
replacement, but there are other tty_operations that don't, like
hvc_poll_put_char(). So if we move that to kdb_msg_write(), then we'll
be adding extra '\r' for other tty types. Would that be a problem?

Thanks,
Omar

Re: kgdb replacing newline with CRLF in custom query response

[Doug Anderson]

Hi,

On Fri, Nov 8, 2024 at 8:17 AM Omar Sandoval <osandov@osandov.com> wrote:
>
> On Fri, Nov 08, 2024 at 07:31:19AM -0800, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@osandov.com> wrote:
> > >
> > > On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > > >
> > > > > Hi everyone,
> > > > >
> > > > > Amal is working on adding a custom query packet to kgdb for getting the
> > > > > kernel's vmcoreinfo. The rationale and details are available here:
> > > > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
> > > > >
> > > > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> > > > > response and doubling the time it takes to transmit over a slow serial
> > > > > connection. Instead, we were hoping to use the escaped binary format,
> > > > > which escapes the characters #$}* and leaves other bytes untouched.
> > > > >
> > > > > We ran into a problem, though: vmcoreinfo contains newline characters,
> > > > > which the serial core replaces with CRLF; see commit c7d44a02ac60
> > > > > ("serial_core: Commonalize crlf when working w/ a non open console
> > > > > port").
> > > >
> > > > FWIW, the problem predates that commit, but that commit at least moved
> > > > it to be someplace common. Before that some serial drivers were
> > > > hardcoding it... ;-)
> > > >
> > > >
> > > > > This effectively corrupts the data and causes a checksum
> > > > > mismatch.
> > > > >
> > > > > We'd love some input on how to work around this, especially from the
> > > > > kgdb maintainers. Here are a few options, in descending order of my
> > > > > preference:
> > > > >
> > > > > 1. Disable the LF -> CRLF replacement while sending binary data.
> > > > > 2. Escape the newlines using some other custom scheme.
> > > > > 3. Give up and hex-encode the response.
> > > >
> > > > I haven't tried prototyping it, but what about moving the LR -> CRLF
> > > > code to kdb_msg_write(). It would be really easy to do this in the
> > > > case where we're doing "dbg_io_ops->write_char()" since we're already
> > > > processing character at a time. It would be harder to do this when
> > > > also sending the output to the various console, but may not _too_
> > > > hard? You could loop searching for "\n" and send all the characters
> > > > before the "\n", then send a "\r", then send the "\n" and all the
> > > > characters up to the next "\n".

Actually, looking at this again, I wonder if we even need to do any
transformation before sending it to the various consoles. Probably
not. I think it's _just_ the write_char() path that needed it?


> > > > If you did this then you'd lose the "\n" to "\r\n" combination in the
> > > > gdb stub, but _probably_ that doesn't matter?
> > >
> > > That sounds reasonable. I was concerned whether this would affect
> > > anything else using the ->poll_put_char() tty operation, but kgdb seems
> > > to be the only user, does that sound right?
> >
> > Right. As far as I can tell, kgdb is the only user of poll_put_char().
>
> Ah, one other concern, though: only uart_poll_put_char() does the CRLF
> replacement, but there are other tty_operations that don't, like
> hvc_poll_put_char(). So if we move that to kdb_msg_write(), then we'll
> be adding extra '\r' for other tty types. Would that be a problem?

I honestly don't know. I guess also it can be noted that if we do it
in kdb_msg_write() then the write_char() path can even take us to
places that don't invoke uart_poll_put_char(). For instance
"ehci-dbgp.c" registers its own io_module...

Bleh. It _probably_ wouldn't be a big deal to do the LF -> CRLF for
all these, but I don't know for sure. I guess worst case you could add
some flag in the "dbg_io_ops" and figure out how to set it just for
UARTs?


-Doug

Re: kgdb replacing newline with CRLF in custom query response

[Daniel Thompson]

On Fri, Nov 08, 2024 at 09:37:32AM -0800, Doug Anderson wrote:
> Hi,
>
> On Fri, Nov 8, 2024 at 8:17 AM Omar Sandoval <osandov@osandov.com> wrote:
> >
> > On Fri, Nov 08, 2024 at 07:31:19AM -0800, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > >
> > > > On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@osandov.com> wrote:
> > > > > >
> > > > > > Hi everyone,
> > > > > >
> > > > > > Amal is working on adding a custom query packet to kgdb for getting the
> > > > > > kernel's vmcoreinfo. The rationale and details are available here:
> > > > > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet
> > > > > >
> > > > > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the
> > > > > > response and doubling the time it takes to transmit over a slow serial
> > > > > > connection. Instead, we were hoping to use the escaped binary format,
> > > > > > which escapes the characters #$}* and leaves other bytes untouched.
> > > > > >
> > > > > > We ran into a problem, though: vmcoreinfo contains newline characters,
> > > > > > which the serial core replaces with CRLF; see commit c7d44a02ac60
> > > > > > ("serial_core: Commonalize crlf when working w/ a non open console
> > > > > > port").
> > > > >
> > > > > FWIW, the problem predates that commit, but that commit at least moved
> > > > > it to be someplace common. Before that some serial drivers were
> > > > > hardcoding it... ;-)
> > > > >
> > > > >
> > > > > > This effectively corrupts the data and causes a checksum
> > > > > > mismatch.
> > > > > >
> > > > > > We'd love some input on how to work around this, especially from the
> > > > > > kgdb maintainers. Here are a few options, in descending order of my
> > > > > > preference:
> > > > > >
> > > > > > 1. Disable the LF -> CRLF replacement while sending binary data.
> > > > > > 2. Escape the newlines using some other custom scheme.
> > > > > > 3. Give up and hex-encode the response.
> > > > >
> > > > > I haven't tried prototyping it, but what about moving the LR -> CRLF
> > > > > code to kdb_msg_write(). It would be really easy to do this in the
> > > > > case where we're doing "dbg_io_ops->write_char()" since we're already
> > > > > processing character at a time. It would be harder to do this when
> > > > > also sending the output to the various console, but may not _too_
> > > > > hard? You could loop searching for "\n" and send all the characters
> > > > > before the "\n", then send a "\r", then send the "\n" and all the
> > > > > characters up to the next "\n".
>
> Actually, looking at this again, I wonder if we even need to do any
> transformation before sending it to the various consoles. Probably
> not. I think it's _just_ the write_char() path that needed it?
>
>
> > > > > If you did this then you'd lose the "\n" to "\r\n" combination in the
> > > > > gdb stub, but _probably_ that doesn't matter?
> > > >
> > > > That sounds reasonable. I was concerned whether this would affect
> > > > anything else using the ->poll_put_char() tty operation, but kgdb seems
> > > > to be the only user, does that sound right?
> > >
> > > Right. As far as I can tell, kgdb is the only user of poll_put_char().
> >
> > Ah, one other concern, though: only uart_poll_put_char() does the CRLF
> > replacement, but there are other tty_operations that don't, like
> > hvc_poll_put_char(). So if we move that to kdb_msg_write(), then we'll
> > be adding extra '\r' for other tty types. Would that be a problem?
>
> I honestly don't know. I guess also it can be noted that if we do it
> in kdb_msg_write() then the write_char() path can even take us to
> places that don't invoke uart_poll_put_char(). For instance
> "ehci-dbgp.c" registers its own io_module...
>
> Bleh. It _probably_ wouldn't be a big deal to do the LF -> CRLF for
> all these, but I don't know for sure. I guess worst case you could add
> some flag in the "dbg_io_ops" and figure out how to set it just for
> UARTs?

I find myself in a similar position to Doug.

It sounds reasonable to move the CR synthesis into kdb_msg_write() but I
also am not certain the other polling backends will handle this correctly.

However I did apply the following patch and run a few tests and it all
looks good. Specifically there are no regressions from the kgdb test
suite although that's unsurprising since that suite exclusively tests
the serial port. I also fired up the kdb,kms polling backends on x86
and verified that I didn't get double line feeds in kdb (I did not).

That means I'd certainly accept patches based on Doug's idea and if it
proves later that we have to revert and add a new dbg_io_ops pointer to
switch the handler between binary and ascii modes then so be it!


Daniel.


diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d94d73e45fb6d..fff1269c55498 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2738,8 +2738,10 @@ static void uart_poll_put_char(struct tty_driver *driver, int line, char ch)
 	if (!port)
 		return;

+#if 0
 	if (ch == '\n')
 		port->ops->poll_put_char(port, '\r');
+#endif
 	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 6a77f1c779c4c..43a7c8ad741ac 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++;
 	}

Re: kgdb replacing newline with CRLF in custom query response

[Omar Sandoval]

On Fri, Nov 15, 2024 at 02:49:33PM +0000, Daniel Thompson wrote:
> On Fri, Nov 08, 2024 at 09:37:32AM -0800, Doug Anderson wrote:

[snip]

> > Bleh. It _probably_ wouldn't be a big deal to do the LF -> CRLF for
> > all these, but I don't know for sure. I guess worst case you could add
> > some flag in the "dbg_io_ops" and figure out how to set it just for
> > UARTs?
> 
> I find myself in a similar position to Doug.
> 
> It sounds reasonable to move the CR synthesis into kdb_msg_write() but I
> also am not certain the other polling backends will handle this correctly.
> 
> However I did apply the following patch and run a few tests and it all
> looks good. Specifically there are no regressions from the kgdb test
> suite although that's unsurprising since that suite exclusively tests
> the serial port. I also fired up the kdb,kms polling backends on x86
> and verified that I didn't get double line feeds in kdb (I did not).
> 
> That means I'd certainly accept patches based on Doug's idea and if it
> proves later that we have to revert and add a new dbg_io_ops pointer to
> switch the handler between binary and ascii modes then so be it!

Perfect, thanks for testing this, Daniel. Amal will include a
preparatory patch along those lines when he sends the VMCOREINFO packet
patch.

Thanks,
Omar