Re: [PATCH v3] kdb: Get rid of custom debug heap allocator
From: Sumit Garg
Date: Tue Jul 13 2021 - 07:24:18 EST
Hi Doug,
Thanks for your review.
On Tue, 13 Jul 2021 at 04:20, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Jul 8, 2021 at 5:25 AM Sumit Garg <sumit.garg@xxxxxxxxxx> 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 "/**" ?
>
Ack.
> > - * 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
I can see "KSYM_NAME_LEN" defined as 128 here [1]. Are you looking at
any other header file?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/kallsyms.h#n18
> 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?
>
Will revert.
>
> All comments above are nits and not terribly important, so:
>
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
Thanks,
-Sumit