linux-kernel - Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

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] [day] [month] [year] [list]
Date:   Mon, 1 Feb 2021 16:27:37 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Zhang <stephenzhangzsd@...il.com>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] kdb: kdb_support: Fix debugging information problem

Hi,

On Sat, Jan 30, 2021 at 3:24 AM Stephen Zhang <stephenzhangzsd@...il.com> wrote:
>
>  int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
>  {
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbgetsymval: symname=%s, symtab=%px\n", symname,
> -                          symtab);
> +       kdb_dbg_printf(AR, "symname=%s, symtab=%px\n", symname, symtab);
>         memset(symtab, 0, sizeof(*symtab));
>         symtab->sym_start = kallsyms_lookup_name(symname);
>         if (symtab->sym_start) {
> -               if (KDB_DEBUG(AR))
> -                       kdb_printf("kdbgetsymval: returns 1, "
> -                                  "symtab->sym_start=0x%lx\n",
> -                                  symtab->sym_start);
> +               kdb_dbg_printf(AR, "returns 1,symtab->sym_start=0x%lx\n",

nit: There used to be a space after the "," and there no longer is.


> +                             symtab->sym_start);

nit: I think it was supposed to be indented 1 more space so it's under "AR".


>  EXPORT_SYMBOL(kdbgetsymval);
> @@ -87,15 +82,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>  #define knt1_size 128          /* must be >= kallsyms table size */
>         char *knt1 = NULL;
>
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbnearsym: addr=0x%lx, symtab=%px\n", addr, symtab);
> +       kdb_dbg_printf(AR, "addr=0x%lx, symtab=%px\n", addr, symtab);
>         memset(symtab, 0, sizeof(*symtab));
>
>         if (addr < 4096)
>                 goto out;
>         knt1 = debug_kmalloc(knt1_size, GFP_ATOMIC);
>         if (!knt1) {
> -               kdb_printf("kdbnearsym: addr=0x%lx cannot kmalloc knt1\n",
> +               kdb_func_printf("addr=0x%lx cannot kmalloc knt1\n",
>                            addr);

nit: "addr" used to be indented properly before your change and now
it's not.  It could also move up to the previous line.


> @@ -147,11 +141,10 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
>
>         if (symtab->mod_name == NULL)
>                 symtab->mod_name = "kernel";
> -       if (KDB_DEBUG(AR))
> -               kdb_printf("kdbnearsym: returns %d symtab->sym_start=0x%lx, "
> -                  "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> -                  symtab->sym_start, symtab->mod_name, symtab->sym_name,
> -                  symtab->sym_name);
> +       kdb_dbg_printf(AR, "returns %d symtab->sym_start=0x%lx, "
> +               "symtab->mod_name=%px, symtab->sym_name=%px (%s)\n", ret,
> +               symtab->sym_start, symtab->mod_name, symtab->sym_name,
> +               symtab->sym_name);

This indention doesn't line up, but it didn't before.  I guess I'd let
Daniel say if he likes this or would prefer different wrapping /
indentation.


> @@ -884,18 +876,17 @@ void debug_kusage(void)
>         if (!debug_kusage_one_time)
>                 goto out;
>         debug_kusage_one_time = 0;
> -       kdb_printf("%s: debug_kmalloc memory leak dah_first %d\n",
> -                  __func__, dah_first);
> +       kdb_func_printf("debug_kmalloc memory leak dah_first %d\n", dah_first);
>         if (dah_first) {
>                 h_used = (struct debug_alloc_header *)debug_alloc_pool;
> -               kdb_printf("%s: h_used %px size %d\n", __func__, h_used,
> +               kdb_func_printf("h_used %px size %d\n", h_used,
>                            h_used->size);

nit: "h_used->size" used to be indented properly before your change
and now it's not.  It could also move up to the previous line.

>         }
>         do {
>                 h_used = (struct debug_alloc_header *)
>                           ((char *)h_free + dah_overhead + h_free->size);
> -               kdb_printf("%s: h_used %px size %d caller %px\n",
> -                          __func__, h_used, h_used->size, h_used->caller);
> +               kdb_func_printf("h_used %px size %d caller %px\n",
> +                         h_used, h_used->size, h_used->caller);

nit: "h_used" used to be indented properly before your change and now it's not.


> @@ -903,8 +894,8 @@ void debug_kusage(void)
>                   ((char *)h_free + dah_overhead + h_free->size);
>         if ((char *)h_used - debug_alloc_pool !=
>             sizeof(debug_alloc_pool_aligned))
> -               kdb_printf("%s: h_used %px size %d caller %px\n",
> -                          __func__, h_used, h_used->size, h_used->caller);
> +               kdb_func_printf("h_used %px size %d caller %px\n",
> +                          h_used, h_used->size, h_used->caller);

nit: "h_used" used to be indented properly before your change and now it's not.


It's possible that Daniel would prefer to fix these word-wrapping and
indention things when he applies your change?

I know the above is all pretty nitty, but given that the whole point
of the change is to clean up the code it seems like it's fair game to
make sure the code touched is fully clean...

-Doug