Message-ID: <CAD=FV=WJsUZ1+v5Rd+Pcx+3tO18ivgax4i8XfRg2BZJ_OEEu+A@mail.gmail.com>
Date: Fri, 25 Oct 2024 15:31:24 -0700
From: Doug Anderson <dianders@...omium.org>
To: Nir Lichtman <nir@...htman.org>
Cc: kgdb-bugreport@...ts.sourceforge.net, linux-trace-kernel@...r.kernel.org, 
	yuran.pereira@...mail.com, jason.wessel@...driver.com, 
	daniel.thompson@...aro.org, rostedt@...dmis.org, mhiramat@...nel.org, 
	linux-kernel@...r.kernel.org, linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v2 1/2] kdb: Replace the use of simple_strto with safer
 kstrto in kdb_main

Hi,

On Mon, Oct 21, 2024 at 2:14 PM Nir Lichtman <nir@...htman.org> wrote:
>
> The simple_str* family of functions perform no error checking in
> scenarios where the input value overflows the intended output variable.
> This results in these functions successfully returning even when the
> output does not match the input string.
>
> Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(),
> simple_strtoul(), and simple_strtoull() functions explicitly ignore
> overflows, which may lead to unexpected results in callers."
> Hence, the use of those functions is discouraged.
>
> This patch replaces all uses of the simple_strto* series of functions
> with their safer kstrto* alternatives.
>
> Side effects of this patch:
> - Every string to long or long long conversion using kstrto* is now
>   checked for failure.
> - kstrto* errors are handled with appropriate `KDB_BADINT` wherever
>   applicable.
> - A good side effect is that we end up saving a few lines of code
>   since unlike in simple_strto* functions, kstrto functions do not
>   need an additional "end pointer" variable, and the return values
>   of the latter can be directly checked in an "if" statement without
>   the need to define additional `ret` or `err` variables.
>   This, of course, results in cleaner, yet still easy to understand
>   code.
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull
>
> Signed-off-by: Yuran Pereira <yuran.pereira@...mail.com>

Since this was Yuran's patch originally and is still substantially the
same, keeping their sign-off first was good. ...but:

* You should have kept Yuran as the "author". This should have
happened automatically in your local "git" repo when you picked
Yuran's patch and then made changes. If it didn't you could manually
set the author with "git commit --amend --author=blah". Even if it's
right in your local repository, though, it won't be right when someone
applies your patch. This is because when someone else is the author
the first line of the commit message is supposed to be "From: Original
Author <their-email@...ress>". Your message doesn't have that so when
I grab it it looks like it came from you. If you use a tool like
"patman" or "b4" to post your patches (or even git-send-email) this is
handled for you.

* When you take someone else's patch and make a small change it's nice
to mention what you changed in the commit message.

If you want to see an example of where I took someone else's patch and
posted it, you can see:

http://lore.kernel.org/r/20241004171340.v2.1.I938c91d10e454e841fdf5d64499a8ae8514dc004@changeid

...though maybe ignore the fact that I managed to accidentally add a
Signed-off-by for both of my email addresses--oops. You can see that
it has a "From: " line that patman/git-send-email added for me and you
can also see my mention of what I did:

[dianders: rebased to modern kernels]


> @@ -402,42 +402,15 @@ static void kdb_printenv(void)
>   */
>  int kdbgetularg(const char *arg, unsigned long *value)
>  {
> -       char *endp;
> -       unsigned long val;
> -
> -       val = simple_strtoul(arg, &endp, 0);
> -
> -       if (endp == arg) {
> -               /*
> -                * Also try base 16, for us folks too lazy to type the
> -                * leading 0x...
> -                */

In my original response [1] I suggested that the removal of the base
16 fallback should probably have been done in a separate patch (in
other words change this from a 2-patch series to a 3-patch series).
That keeps the "cleanup" patch separate from the one that's changing
functionality. It's not a huge deal but it would be nice if you could
split it out.

If, for some reason, you didn't split it out then you'd certainly have
wanted to mention it in the commit message of the patch since it's a
change in functionality.

[1] https://lore.kernel.org/r/CAD=FV=VZ61XFb1Ks79BHr1jL1jwf_36wYXryy0ZXOz1xTQ9zOg@mail.gmail.com/

Powered by blists - more mailing lists