linux-kernel - Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Aug 2021 11:49:57 -0600
From:   jim.cromie@...il.com
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks

> >
> > There is still plenty of bikeshedding to do.
>
> > ---
> > v4+:
> >
> > . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> > . in query, replace hardcoded "i915" w kp->mod->name
> > . static inline the stubs
> > . const *str in structs, const array. -Emil
> > . dyndbg: add do-nothing DEFINE_DYNAMIC_DEBUG_CATEGORIES if !DD_CORE
> > . call MOD_PARM_DESC(name, "$desc") for users
> > . simplify callback, remove bit-change detection
> > . config errs reported by <lkp@...el.com>
> >
> > ddh-helpers
>
> > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
>
> So, it is signed or not? I didn't get (perhaps due to misplaced changlog?).
>

It might be my --- snip, and reliance on format-patch -s

would it work if I used --  2char snip ?

> ...
>
> >  } __attribute__((aligned(8)));
> >
> >
>
> Do we need two blank lines here?
>
> > +struct kernel_param;
>
> ...
>
> > +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> > +{
> > +     unsigned long inbits;
> > +     int rc, i, chgct = 0, totct = 0;
> > +     char query[OUR_QUERY_SIZE];
> > +     struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
>
> So you need space after ')' ?
>
> > +     rc = kstrtoul(instr, 0, &inbits);
> > +     if (rc) {
> > +             pr_err("set_dyndbg: failed\n");
>
> > +             return -EINVAL;
>
> Why not to return rc?
>
> > +     }
> > +     vpr_info("set_dyndbg: input 0x%lx\n", inbits);
> > +
> > +     for (i = 0; !!bitmap[i].prefix; i++) {
>
> Hmm... Why not simply
>
>         for (bitmap = ...; bitmap->prefix; bitmap++) {
>
> ?
>
> > +
>
> Redundant blank line.
>
> > +             sprintf(query, "format '^%s' %cp", bitmap[i].prefix,
> > +                     test_bit(i, &inbits) ? '+' : '-');
>
> snprintf() ?
>
> > +
> > +             chgct = dynamic_debug_exec_queries(query, kp->mod->name);
> > +
> > +             v2pr_info("bit-%d: %d changes by '%s'\n", i, chgct, query);
> > +             totct += chgct;
> > +     }
> > +     vpr_info("total changes: %d\n", totct);
> > +     return 0;
> > +}
>
> ...
>
> > +     return scnprintf(buffer, PAGE_SIZE, "%u\n",
> > +                      *((unsigned int *)kp->arg));
>
> One line.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>