Messages in this thread Patch in this message | | | From | Doug Anderson <> | Date | Mon, 12 Jul 2021 15:49:56 -0700 | Subject | Re: [PATCH v3] kdb: Get rid of custom debug heap allocator |
| |
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>
| |