From: Doug Anderson <dianders@chromium.org>
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>
Subject: Re: [PATCH v3] kdb: Get rid of custom debug heap allocator
Date: Mon, 12 Jul 2021 15:49:56 -0700 [thread overview]
Message-ID: <CAD=FV=UhL32e7spQjr38Lwng0D7mUK+R7iGnmBah=tXzzXsO5g@mail.gmail.com> (raw)
In-Reply-To: <20210708122447.3880803-1-sumit.garg@linaro.org>
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>
next prev parent reply other threads:[~2021-07-12 22:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-08 12:24 [PATCH v3] kdb: Get rid of custom debug heap allocator Sumit Garg
2021-07-12 22:49 ` Doug Anderson [this message]
2021-07-13 11:24 ` Sumit Garg
2021-07-13 13:45 ` Doug Anderson
2021-07-13 15:10 ` Daniel Thompson
2021-07-13 15:12 ` Doug Anderson
2021-07-14 5:03 ` Sumit Garg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=UhL32e7spQjr38Lwng0D7mUK+R7iGnmBah=tXzzXsO5g@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=daniel.thompson@linaro.org \
--cc=jason.wessel@windriver.com \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=sumit.garg@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.