linux-kernel - Re: [PATCH] kdb: Refactor env variables get/set code

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 09:14:28 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Sumit Garg <sumit.garg@...aro.org>
Cc:     kgdb-bugreport@...ts.sourceforge.net,
        Jason Wessel <jason.wessel@...driver.com>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kdb: Refactor env variables get/set code

Hi,

On Mon, Jan 25, 2021 at 6:30 AM Sumit Garg <sumit.garg@...aro.org> wrote:
>
> diff --git a/kernel/debug/kdb/kdb_env.c b/kernel/debug/kdb/kdb_env.c
> new file mode 100644
> index 0000000..33ab5e6
> --- /dev/null
> +++ b/kernel/debug/kdb/kdb_env.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kernel Debugger Architecture Independent Environment Functions
> + *
> + * Copyright (c) 1999-2004 Silicon Graphics, Inc.  All Rights Reserved.
> + * Copyright (c) 2009 Wind River Systems, Inc.  All Rights Reserved.
> + * 03/02/13    added new 2.5 kallsyms <xavier.bru@...l.net>

I'm not sure the policy for copying over copyright notices like this,
but I would have expected them to get copied over from the file you
got the code from?  These are slightly different.

> + */
> +
> +#include <linux/kdb.h>
> +#include <linux/string.h>
> +#include "kdb_private.h"
> +
> +/*
> + * Initial environment.   This is all kept static and local to
> + * this file.   We don't want to rely on the memory allocation
> + * mechanisms in the kernel, so we use a very limited allocate-only
> + * heap for new and altered environment variables.  The entire
> + * environment is limited to a fixed number of entries (add more
> + * to __env[] if required) and a fixed amount of heap (add more to
> + * KDB_ENVBUFSIZE if required).
> + */
> +static char *__env[] = {
> +#if defined(CONFIG_SMP)
> +       "PROMPT=[%d]kdb> ",
> +#else
> +       "PROMPT=kdb> ",
> +#endif
> +       "MOREPROMPT=more> ",
> +       "RADIX=16",
> +       "MDCOUNT=8",            /* lines of md output */
> +       KDB_PLATFORM_ENV,
> +       "DTABCOUNT=30",
> +       "NOSECT=1",
> +       (char *)0,

In a follow-up patch, I guess these could move from 0 to NULL and
remove the cast?


> +/*
> + * kdbgetenv - This function will return the character string value of
> + *     an environment variable.
> + * Parameters:
> + *     match   A character string representing an environment variable.
> + * Returns:
> + *     NULL    No environment variable matches 'match'
> + *     char*   Pointer to string value of environment variable.
> + */

In a follow-up patch, the above could be moved to kernel-doc format,
which we're trying to move to for kgdb when we touch code.

I will leave it up to you about whether the new functions introduced
in this patch are introduced with the proper kernel doc format or move
to the right format in the same follow-up patch.


> +/*
> + * kdb_prienv - Display the current environment variables.
> + */
> +void kdb_prienv(void)

IMO saving the two characters in the function name isn't worth it,
especially since this function is called in only one place.  Use
kdb_printenv()

-Doug