* [PATCH 0/2] dyndbg: 2 fixes/cleanups for 5.9
@ 2020-09-21 19:04 Jim Cromie
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
2020-09-21 19:04 ` [PATCH 2/2] dyndbg: use keyword, arg varnames for query term pairs Jim Cromie
0 siblings, 2 replies; 8+ messages in thread
From: Jim Cromie @ 2020-09-21 19:04 UTC (permalink / raw)
To: jbaron, gregkh, linux-kernel; +Cc: Jim Cromie
2 things which might qualify as fixes
a bad (reverted) patch hit a BUG_ON, change that to return -EINVAL instead
keep useful part of the reverted patch, use keyword, arg varnames
Jim Cromie (2):
dyndbg: dont panic over bad input
dyndbg: use keyword, arg varnames for query term pairs
lib/dynamic_debug.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-21 19:04 [PATCH 0/2] dyndbg: 2 fixes/cleanups for 5.9 Jim Cromie
@ 2020-09-21 19:04 ` Jim Cromie
2020-09-21 19:29 ` Joe Perches
` (2 more replies)
2020-09-21 19:04 ` [PATCH 2/2] dyndbg: use keyword, arg varnames for query term pairs Jim Cromie
1 sibling, 3 replies; 8+ messages in thread
From: Jim Cromie @ 2020-09-21 19:04 UTC (permalink / raw)
To: jbaron, gregkh, linux-kernel; +Cc: Jim Cromie, Petr Mladek
This BUG_ON, from 2009, caught the impossible case of a word-char both
starting and ending a string (loosely speaking). A bad (reverted)
patch finally hit this case, but even "impossibly bad input" is no
reason to panic the kernel. Instead pr_err and return -EINVAL.
Reported-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
lib/dynamic_debug.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 2d4dfd44b0fa5..90ddf07ce34fe 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
} else {
for (end = buf; *end && !isspace(*end); end++)
;
- BUG_ON(end == buf);
+ if (end == buf) {
+ pr_err("expected non-empty bareword");
+ return -EINVAL;
+ }
}
/* `buf' is start of word, `end' is one past its end */
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] dyndbg: use keyword, arg varnames for query term pairs
2020-09-21 19:04 [PATCH 0/2] dyndbg: 2 fixes/cleanups for 5.9 Jim Cromie
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
@ 2020-09-21 19:04 ` Jim Cromie
1 sibling, 0 replies; 8+ messages in thread
From: Jim Cromie @ 2020-09-21 19:04 UTC (permalink / raw)
To: jbaron, gregkh, linux-kernel; +Cc: Jim Cromie
optimize for clarity by replacing word[i,i+1] refs with temps.
no functional changes.
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
lib/dynamic_debug.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 90ddf07ce34fe..23e7dd967b0e6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -387,10 +387,13 @@ static int ddebug_parse_query(char *words[], int nwords,
query->module = modname;
for (i = 0; i < nwords; i += 2) {
- if (!strcmp(words[i], "func")) {
- rc = check_set(&query->function, words[i+1], "func");
- } else if (!strcmp(words[i], "file")) {
- if (check_set(&query->filename, words[i+1], "file"))
+ char *keyword = words[i];
+ char *arg = words[i+1];
+
+ if (!strcmp(keyword, "func")) {
+ rc = check_set(&query->function, arg, "func");
+ } else if (!strcmp(keyword, "file")) {
+ if (check_set(&query->filename, arg, "file"))
return -EINVAL;
/* tail :$info is function or line-range */
@@ -406,18 +409,18 @@ static int ddebug_parse_query(char *words[], int nwords,
if (parse_linerange(query, fline))
return -EINVAL;
}
- } else if (!strcmp(words[i], "module")) {
- rc = check_set(&query->module, words[i+1], "module");
- } else if (!strcmp(words[i], "format")) {
- string_unescape_inplace(words[i+1], UNESCAPE_SPACE |
+ } else if (!strcmp(keyword, "module")) {
+ rc = check_set(&query->module, arg, "module");
+ } else if (!strcmp(keyword, "format")) {
+ string_unescape_inplace(arg, UNESCAPE_SPACE |
UNESCAPE_OCTAL |
UNESCAPE_SPECIAL);
- rc = check_set(&query->format, words[i+1], "format");
- } else if (!strcmp(words[i], "line")) {
- if (parse_linerange(query, words[i+1]))
+ rc = check_set(&query->format, arg, "format");
+ } else if (!strcmp(keyword, "line")) {
+ if (parse_linerange(query, arg))
return -EINVAL;
} else {
- pr_err("unknown keyword \"%s\"\n", words[i]);
+ pr_err("unknown keyword \"%s\"\n", keyword);
return -EINVAL;
}
if (rc)
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
@ 2020-09-21 19:29 ` Joe Perches
2020-09-22 3:57 ` jim.cromie
2020-09-22 8:08 ` Petr Mladek
2020-09-27 12:29 ` Greg KH
2 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2020-09-21 19:29 UTC (permalink / raw)
To: Jim Cromie, jbaron, gregkh, linux-kernel; +Cc: Petr Mladek
On Mon, 2020-09-21 at 13:04 -0600, Jim Cromie wrote:
> This BUG_ON, from 2009, caught the impossible case of a word-char both
> starting and ending a string (loosely speaking). A bad (reverted)
> patch finally hit this case, but even "impossibly bad input" is no
> reason to panic the kernel. Instead pr_err and return -EINVAL.
[]
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
[]
> @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> } else {
> for (end = buf; *end && !isspace(*end); end++)
> ;
> - BUG_ON(end == buf);
> + if (end == buf) {
> + pr_err("expected non-empty bareword");
missing newline
This message is also unintelligible.
What is a non-empty bareword?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-21 19:29 ` Joe Perches
@ 2020-09-22 3:57 ` jim.cromie
0 siblings, 0 replies; 8+ messages in thread
From: jim.cromie @ 2020-09-22 3:57 UTC (permalink / raw)
To: Joe Perches; +Cc: Jason Baron, Greg KH, LKML, Petr Mladek
On Mon, Sep 21, 2020 at 1:29 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-09-21 at 13:04 -0600, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking). A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel. Instead pr_err and return -EINVAL.
> []
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> []
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> > } else {
> > for (end = buf; *end && !isspace(*end); end++)
> > ;
> > - BUG_ON(end == buf);
> > + if (end == buf) {
> > + pr_err("expected non-empty bareword");
>
> missing newline
>
> This message is also unintelligible.
> What is a non-empty bareword?
>
>
hmm, I borrowed the term from perl
en.wiktionary.org › wiki › bareword
(programming, chiefly Perl) A sequence of text characters in source
code that do not form a keyword nor part of a quoted string, and may
potentially be interpreted ...
basically, a not-quoted word, a keyword or not-quoted-value
Im open that there might be better terminology.
have any suggestions ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
2020-09-21 19:29 ` Joe Perches
@ 2020-09-22 8:08 ` Petr Mladek
2020-09-22 14:23 ` jim.cromie
2020-09-27 12:29 ` Greg KH
2 siblings, 1 reply; 8+ messages in thread
From: Petr Mladek @ 2020-09-22 8:08 UTC (permalink / raw)
To: Jim Cromie; +Cc: jbaron, gregkh, linux-kernel
On Mon 2020-09-21 13:04:32, Jim Cromie wrote:
> This BUG_ON, from 2009, caught the impossible case of a word-char both
> starting and ending a string (loosely speaking). A bad (reverted)
> patch finally hit this case, but even "impossibly bad input" is no
> reason to panic the kernel. Instead pr_err and return -EINVAL.
>
> Reported-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> lib/dynamic_debug.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> } else {
> for (end = buf; *end && !isspace(*end); end++)
> ;
> - BUG_ON(end == buf);
> + if (end == buf) {
> + pr_err("expected non-empty bareword");
> + return -EINVAL;
My understanding is that the BUG_ON() was there to catch eventual bugs
in the algorithm.
IMHO, it was never reachable in the original code:
1. The following lines will skip all spaces and bail out
when we reached the trailing '\0':
buf = skip_spaces(buf);
if (!*buf)
break; /* oh, it was trailing whitespace */
2. The following code will find the end of a quoted text:
if (*buf == '"' || *buf == '\'') {
int quote = *buf++;
for (end = buf; *end && *end != quote; end++)
3. The else part will find end of non-quoted word:
} else {
for (end = buf; *end && !isspace(*end); end++)
4. The BUG_ON() will trigger when the above cycle reached the
trailing '\0' or space.
This will never happen because this situation was caught
in the 1st step.
Your patch triggered the BUG_ON() because it wanted to handle
'=' as a special character and was incomplete.
If you want to handle '=' special way. You need to do it the same way
as with the space. You need to skip it in 1st step. And it must mark
the end of the word in 4th step.
But it will be more complicated. You must be able to handle
mix of spaces and '='. I mean the following situations:
word=word
word =word
word= word
word = word
word = = word /* failure ? */
Back to the BUG_ON(). It might be changed to something like:
pr_err("Internal error when parsing dynamic debug query\n");
return -EINVAL;
Best Regards,
Petr
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-22 8:08 ` Petr Mladek
@ 2020-09-22 14:23 ` jim.cromie
0 siblings, 0 replies; 8+ messages in thread
From: jim.cromie @ 2020-09-22 14:23 UTC (permalink / raw)
To: Petr Mladek; +Cc: Jason Baron, Greg KH, LKML
On Tue, Sep 22, 2020 at 2:08 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2020-09-21 13:04:32, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking). A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel. Instead pr_err and return -EINVAL.
> >
> > Reported-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> > lib/dynamic_debug.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> > } else {
> > for (end = buf; *end && !isspace(*end); end++)
> > ;
> > - BUG_ON(end == buf);
> > + if (end == buf) {
> > + pr_err("expected non-empty bareword");
> > + return -EINVAL;
>
> My understanding is that the BUG_ON() was there to catch eventual bugs
> in the algorithm.
>
> IMHO, it was never reachable in the original code:
>
> 1. The following lines will skip all spaces and bail out
> when we reached the trailing '\0':
>
> buf = skip_spaces(buf);
> if (!*buf)
> break; /* oh, it was trailing whitespace */
>
>
> 2. The following code will find the end of a quoted text:
>
> if (*buf == '"' || *buf == '\'') {
> int quote = *buf++;
> for (end = buf; *end && *end != quote; end++)
>
>
> 3. The else part will find end of non-quoted word:
>
> } else {
> for (end = buf; *end && !isspace(*end); end++)
>
> 4. The BUG_ON() will trigger when the above cycle reached the
> trailing '\0' or space.
>
> This will never happen because this situation was caught
> in the 1st step.
>
>
> Your patch triggered the BUG_ON() because it wanted to handle
> '=' as a special character and was incomplete.
>
> If you want to handle '=' special way. You need to do it the same way
> as with the space. You need to skip it in 1st step. And it must mark
> the end of the word in 4th step.
>
> But it will be more complicated. You must be able to handle
> mix of spaces and '='. I mean the following situations:
>
> word=word
> word =word
> word= word
> word = word
> word = = word /* failure ? */
>
>
> Back to the BUG_ON(). It might be changed to something like:
>
> pr_err("Internal error when parsing dynamic debug query\n");
> return -EINVAL;
>
>
> Best Regards,
> Petr
yes, the original patch was ill conceived
Im blaming Transient Acute Myopia,
where I couldnt see to the end of the line, where "=flags"
is a proper flag setting.
That "interferes" with using '=' to separate keyword=value.
As you outlined, its possible to implement something that handles both,
but I decided that handling keyword=value is just a convenience feature,
and isnt worth the added corner-cases and explanatory burden.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dyndbg: dont panic over bad input
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
2020-09-21 19:29 ` Joe Perches
2020-09-22 8:08 ` Petr Mladek
@ 2020-09-27 12:29 ` Greg KH
2 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2020-09-27 12:29 UTC (permalink / raw)
To: Jim Cromie; +Cc: jbaron, linux-kernel, Petr Mladek
On Mon, Sep 21, 2020 at 01:04:32PM -0600, Jim Cromie wrote:
> This BUG_ON, from 2009, caught the impossible case of a word-char both
> starting and ending a string (loosely speaking). A bad (reverted)
> patch finally hit this case, but even "impossibly bad input" is no
> reason to panic the kernel. Instead pr_err and return -EINVAL.
>
> Reported-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> lib/dynamic_debug.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> } else {
> for (end = buf; *end && !isspace(*end); end++)
> ;
> - BUG_ON(end == buf);
> + if (end == buf) {
> + pr_err("expected non-empty bareword");
Need a "\n" here, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-27 12:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21 19:04 [PATCH 0/2] dyndbg: 2 fixes/cleanups for 5.9 Jim Cromie
2020-09-21 19:04 ` [PATCH 1/2] dyndbg: dont panic over bad input Jim Cromie
2020-09-21 19:29 ` Joe Perches
2020-09-22 3:57 ` jim.cromie
2020-09-22 8:08 ` Petr Mladek
2020-09-22 14:23 ` jim.cromie
2020-09-27 12:29 ` Greg KH
2020-09-21 19:04 ` [PATCH 2/2] dyndbg: use keyword, arg varnames for query term pairs Jim Cromie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.