Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210708122447.3880803-1-sumit.garg@linaro.org>
In-Reply-To: <20210708122447.3880803-1-sumit.garg@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 12 Jul 2021 15:49:56 -0700
Message-ID: <CAD=FV=UhL32e7spQjr38Lwng0D7mUK+R7iGnmBah=tXzzXsO5g@mail.gmail.com>
Subject: Re: [PATCH v3] kdb: Get rid of custom debug heap allocator
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jason Wessel <jason.wessel@windriver.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Thu, Jul 8, 2021 at 5:25 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> @@ -233,10 +232,6 @@ extern struct task_struct *kdb_curr_task(int);
>
> @@ -52,48 +52,48 @@ int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
>  }
>  EXPORT_SYMBOL(kdbgetsymval);
>
> -static char *kdb_name_table[100];      /* arbitrary size */
> -
>  /*
> - * kdbnearsym -        Return the name of the symbol with the nearest address

nit: This is now kernel-doc, right? So start with "/**" ?

> - *     less than 'addr'.
> + * kdbnearsym() - Return the name of the symbol with the nearest address
> + *                less than @addr.
> + * @addr: Address to check for near symbol
> + * @symtab: Structure to receive results
>   *
> - * Parameters:
> - *     addr    Address to check for symbol near
> - *     symtab  Structure to receive results
> - * Returns:
> - *     0       No sections contain this address, symtab zero filled
> - *     1       Address mapped to module/symbol/section, data in symtab
> - * Remarks:
> - *     2.6 kallsyms has a "feature" where it unpacks the name into a
> - *     string.  If that string is reused before the caller expects it
> - *     then the caller sees its string change without warning.  To
> - *     avoid cluttering up the main kdb code with lots of kdb_strdup,
> - *     tests and kfree calls, kdbnearsym maintains an LRU list of the
> - *     last few unique strings.  The list is sized large enough to
> - *     hold active strings, no kdb caller of kdbnearsym makes more
> - *     than ~20 later calls before using a saved value.
> + * WARNING: This function may return a pointer to a single statically
> + * allocated buffer (namebuf). kdb's unusual calling context (single
> + * threaded, all other CPUs halted) provides us sufficient locking for
> + * this to be safe. The only constraint imposed by the static buffer is
> + * that the caller must consume any previous reply prior to another call
> + * to lookup a new symbol.
> + *
> + * Note that, strictly speaking, some architectures may re-enter the kdb
> + * trap if the system turns out to be very badly damaged and this breaks
> + * the single-threaded assumption above. In these circumstances successful
> + * continuation and exit from the inner trap is unlikely to work and any
> + * user attempting this receives a prominent warning before being allowed
> + * to progress. In these circumstances we remain memory safe because
> + * namebuf[KSYM_NAME_LEN-1] will never change from '\0' although we do
> + * tolerate the possibility of garbled symbol display from the outer kdb
> + * trap.
> + *
> + * Return:
> + * * 0 - No sections contain this address, symtab zero filled
> + * * 1 - Address mapped to module/symbol/section, data in symtab
>   */
>  int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>  {
>         int ret = 0;
>         unsigned long symbolsize = 0;
>         unsigned long offset = 0;
> -#define knt1_size 128          /* must be >= kallsyms table size */
> -       char *knt1 = NULL;
> +       static char namebuf[KSYM_NAME_LEN];

I guess this also ends up fixing a bug too, right? My greps show that
"KSYM_NAME_LEN" is 512 and the first thing kallsyms_lookup() does it
zero out byte 511. Previously we were only allocating 128 bytes so I
guess we were writing past the end.

Assuming I understood correctly, maybe mention the bugfix in the commit text?


> @@ -102,63 +102,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>         symtab->sym_end = symtab->sym_start + symbolsize;
>         ret = symtab->sym_name != NULL && *(symtab->sym_name) != '\0';
>
> -       if (ret) {
> -               int i;
> -               /* Another 2.6 kallsyms "feature".  Sometimes the sym_name is
> -                * set but the buffer passed into kallsyms_lookup is not used,
> -                * so it contains garbage.  The caller has to work out which
> -                * buffer needs to be saved.
> -                *
> -                * What was Rusty smoking when he wrote that code?
> -                */
> -               if (symtab->sym_name != knt1) {
> -                       strncpy(knt1, symtab->sym_name, knt1_size);
> -                       knt1[knt1_size-1] = '\0';
> -               }
> -               for (i = 0; i < ARRAY_SIZE(kdb_name_table); ++i) {
> -                       if (kdb_name_table[i] &&
> -                           strcmp(kdb_name_table[i], knt1) == 0)
> -                               break;
> -               }
> -               if (i >= ARRAY_SIZE(kdb_name_table)) {
> -                       debug_kfree(kdb_name_table[0]);
> -                       memmove(kdb_name_table, kdb_name_table+1,
> -                              sizeof(kdb_name_table[0]) *
> -                              (ARRAY_SIZE(kdb_name_table)-1));
> -               } else {
> -                       debug_kfree(knt1);
> -                       knt1 = kdb_name_table[i];
> -                       memmove(kdb_name_table+i, kdb_name_table+i+1,
> -                              sizeof(kdb_name_table[0]) *
> -                              (ARRAY_SIZE(kdb_name_table)-i-1));
> -               }
> -               i = ARRAY_SIZE(kdb_name_table) - 1;
> -               kdb_name_table[i] = knt1;
> -               symtab->sym_name = kdb_name_table[i];
> -               knt1 = NULL;

I definitely had a hard time following exactly what all the cases were
handling and if they were all right, but I agree that we can kill the
old code (yay!)


> @@ -249,6 +200,7 @@ void kdb_symbol_print(unsigned long addr, const kdb_symtab_t *symtab_p,
>                       unsigned int punc)
>  {
>         kdb_symtab_t symtab, *symtab_p2;
> +
>         if (symtab_p) {
>                 symtab_p2 = (kdb_symtab_t *)symtab_p;
>         } else {

nit: unrelated whitespace change?


All comments above are nits and not terribly important, so:

Reviewed-by: Douglas Anderson <dianders@chromium.org>