| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD=FV=W9Tdh2hPekzgSYnCqoTX_ms1GY-FXgnxd-uEW0SWbyuw@mail.gmail.com>
Date: Tue, 8 Oct 2019 15:21:02 -0700
From: Doug Anderson <dianders@...omium.org>
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: Jason Wessel <jason.wessel@...driver.com>,
kgdb-bugreport@...ts.sourceforge.net,
LKML <linux-kernel@...r.kernel.org>,
Patch Tracking <patches@...aro.org>
Subject: Re: [PATCH v2 3/5] kdb: Remove special case logic from kdb_read()
Hi,
On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> kdb_read() contains special case logic to force it exit after reading
> a single character. We can remove all the special case logic by directly
> calling the function to read a single character instead. This also
> allows us to tidy up the function prototype which, because it now matches
> getchar(), we can also rename in order to make its role clearer.
nit: since you're doing the rename, should you rename
kdb_read_handle_escape() to match?
> Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
> ---
> kernel/debug/kdb/kdb_io.c | 56 ++++++++++++++++-----------------------
> 1 file changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 78cb6e339408..a9e73bc9d1c3 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -106,7 +106,19 @@ static int kdb_read_handle_escape(char *buf, size_t sz)
> return -1;
> }
>
> -static int kdb_read_get_key(char *buffer, size_t bufsize)
> +/*
> + * kdb_getchar
> + *
> + * Read a single character from kdb console (or consoles).
nit: should we start moving to the standard kernel convention of
kernel-doc style comments? See
"Documentation/doc-guide/kernel-doc.rst"
> + *
> + * An escape key could be the start of a vt100 control sequence such as \e[D
> + * (left arrow) or it could be a character in its own right. The standard
> + * method for detecting the difference is to wait for 2 seconds to see if there
> + * are any other characters. kdb is complicated by the lack of a timer service
> + * (interrupts are off), by multiple input sources. Escape sequence processing
> + * has to be done as states in the polling loop.
Before your paragraph, maybe add: "Most of the work of this function
is dealing with escape sequences." to give it a little bit of context.
> + */
> +static int kdb_getchar(void)
Is "int" the right return type here, or "unsigned char"? You never
return EOF, right? Always a valid character? NOTE: if you do change
this to "unsigned char" I think you still need to keep the local "key"
variable as an "int" since -1 shouldn't be confused with the character
255.
> {
> #define ESCAPE_UDELAY 1000
> #define ESCAPE_DELAY (2*1000000/ESCAPE_UDELAY) /* 2 seconds worth of udelays */
> @@ -124,7 +136,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> }
>
> key = (*f)();
> -
> if (key == -1) {
> if (escape_delay) {
> udelay(ESCAPE_UDELAY);
> @@ -134,14 +145,6 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> continue;
> }
>
> - if (bufsize <= 2) {
> - if (key == '\r')
> - key = '\n';
> - *buffer++ = key;
> - *buffer = '\0';
> - return -1;
> - }
> -
> if (escape_delay == 0 && key == '\e') {
> escape_delay = ESCAPE_DELAY;
> ped = escape_data;
> @@ -183,17 +186,7 @@ static int kdb_read_get_key(char *buffer, size_t bufsize)
> * function. It is not reentrant - it relies on the fact
> * that while kdb is running on only one "master debug" cpu.
> * Remarks:
> - *
> - * The buffer size must be >= 2. A buffer size of 2 means that the caller only
> - * wants a single key.
By removing this you broke "BTAPROMPT". So doing:
set BTAPROMPT=1
bta
It's now impossible to quit out. Not that I've ever used BTAPROMPT,
but seems like we should either get rid of it or keep it working.
> - *
> - * An escape key could be the start of a vt100 control sequence such as \e[D
> - * (left arrow) or it could be a character in its own right. The standard
> - * method for detecting the difference is to wait for 2 seconds to see if there
> - * are any other characters. kdb is complicated by the lack of a timer service
> - * (interrupts are off), by multiple input sources and by the need to sometimes
> - * return after just one key. Escape sequence processing has to be done as
> - * states in the polling loop.
> + * The buffer size must be >= 2.
> */
>
> static char *kdb_read(char *buffer, size_t bufsize)
> @@ -228,9 +221,7 @@ static char *kdb_read(char *buffer, size_t bufsize)
> *cp = '\0';
> kdb_printf("%s", buffer);
> poll_again:
> - key = kdb_read_get_key(buffer, bufsize);
> - if (key == -1)
> - return buffer;
> + key = kdb_getchar();
> if (key != 9)
> tab = 0;
> switch (key) {
> @@ -741,7 +732,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>
> /* check for having reached the LINES number of printed lines */
> if (kdb_nextline >= linecount) {
> - char buf1[16] = "";
> + char ch;
The type of "ch" should be the same as returned by kdb_getchar()?
Either "int" if you're keeping it "int" or "unsigned char"?
> /* Watch out for recursion here. Any routine that calls
> * kdb_printf will come back through here. And kdb_read
> @@ -776,39 +767,38 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> if (logging)
> printk("%s", moreprompt);
>
> - kdb_read(buf1, 2); /* '2' indicates to return
> - * immediately after getting one key. */
> + ch = kdb_getchar();
> kdb_nextline = 1; /* Really set output line 1 */
>
> /* empty and reset the buffer: */
> kdb_buffer[0] = '\0';
> next_avail = kdb_buffer;
> size_avail = sizeof(kdb_buffer);
> - if ((buf1[0] == 'q') || (buf1[0] == 'Q')) {
> + if ((ch == 'q') || (ch == 'Q')) {
> /* user hit q or Q */
> KDB_FLAG_SET(CMD_INTERRUPT); /* command interrupted */
> KDB_STATE_CLEAR(PAGER);
> /* end of command output; back to normal mode */
> kdb_grepping_flag = 0;
> kdb_printf("\n");
> - } else if (buf1[0] == ' ') {
> + } else if (ch == ' ') {
> kdb_printf("\r");
> suspend_grep = 1; /* for this recursion */
> - } else if (buf1[0] == '\n') {
> + } else if (ch == '\n' || ch == '\r') {
> kdb_nextline = linecount - 1;
> kdb_printf("\r");
> suspend_grep = 1; /* for this recursion */
> - } else if (buf1[0] == '/' && !kdb_grepping_flag) {
> + } else if (ch == '/' && !kdb_grepping_flag) {
> kdb_printf("\r");
> kdb_getstr(kdb_grep_string, KDB_GREP_STRLEN,
> kdbgetenv("SEARCHPROMPT") ?: "search> ");
> *strchrnul(kdb_grep_string, '\n') = '\0';
> kdb_grepping_flag += KDB_GREPPING_FLAG_SEARCH;
> suspend_grep = 1; /* for this recursion */
> - } else if (buf1[0] && buf1[0] != '\n') {
> + } else if (ch && ch != '\n') {
Remove "&& ch != '\n'". We would have hit an earlier case in the
if/else anyway. If you really want to keep it here for some reason, I
guess you should also handle '\r' ?
-Doug
Powered by blists - more mailing lists