Re: [PATCH v6 19/24] dyndbg: pin class param storage to u32

On Tue, Jul 7, 2026 at 8:37 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Support for classmaps with more than 32 classes is broken, causing undefined behavior on 32-bit architectures and silent truncation on 64-bit architectures.
> --
>
> --- Patch [19]: [PATCH v6 19/24] dyndbg: pin class param storage to u32 ---
> commit df6ebc9b56fab43581ec7860d30aab6f33272ee2
> Author: Jim Cromie <[email protected]>
>
> dyndbg: pin class param storage to u32
>
> This commit changes the storage of dyndbg class parameters from unsigned long
> to u32 for consistency across architectures. It also shrinks __drm_debug back
> to u32.
>
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index d99c69b9ad12f..af05f4ae3b55e 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> [ ... ]
> > @@ -643,24 +643,27 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
> >       int bi, ct;
> >
> >       if (*new_bits != old_bits)
> > -             v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
> > +             v2pr_info("apply bitmap: 0x%x to: 0x%x for %s\n", *new_bits,
> >                         old_bits, query_modname ?: "'*'");
> >
> >       for (bi = 0; bi < map->length; bi++) {
> > -             if (test_bit(bi, new_bits) == test_bit(bi, &old_bits))
> > +             bool new_b = !!(*new_bits & BIT(bi));
> > +             bool old_b = !!(old_bits & BIT(bi));
>
> [Severity: Medium]
> Since classmaps can theoretically contain up to 63 classes (as mentioned in
> the commit message), is there a risk of an out-of-bounds shift on 32-bit
> architectures?
>

A macro in a later patch (not in this series) limits a classmap to 32 classes,
which is a practical limit for usability of classmap params.
As a case in point, drm.debug has only ~10 DRM_UT_* values.

64 is a per-module class-id space, allowing modules to have several classmaps.
a classmap's base/offset lets it map its 0..31 range into upper class-ids,
which is how a module can have multiple, non-overlapping mappings.


#define __DYNAMIC_DEBUG_CLASSMAP_CHECK(_clnames, _base, _mapty)         \
        static_assert(((_base) >= 0 && (_base) < _DPRINTK_CLASS_DFLT),  \
                      "_base must be in 0..62");                        \
        static_assert(__DDEBUG_ARRAY_SIZE(_clnames) > 0,
                 \
                      "classnames array size must be > 0");             \
        static_assert(__DDEBUG_ARRAY_SIZE(_clnames) <= 32,
         \
                      "classnames array size must be <= 32");           \
        static_assert((__DDEBUG_ARRAY_SIZE(_clnames) + (_base)) <
_DPRINTK_CLASS_DFLT, \
                      "_base + classnames.length must be <= 62");       \
        static_assert(((_mapty) >= DD_CLASS_TYPE_DISJOINT_BITS) &&      \
                      ((_mapty) <= DD_CLASS_TYPE_LEVEL_NUM),            \
                      "unknown class_map_type")

So while its currently possible to define a problematic classmap,
that will not be possible soon.


> BIT(bi) expands to 1UL << bi, so if map->length is greater than 32, this
> shift would exceed the capacity of an unsigned long on 32-bit systems.
>
> On 64-bit systems, *new_bits is now a u32. When it is combined with BIT(bi)
> using a bitwise AND, the u32 is zero-extended. Will this cause new_b and
> old_b to always evaluate to 0 for classes >= 32, silently ignoring the
> requested state?
>
> [ ... ]
> > @@ -704,24 +707,24 @@ static int param_set_dyndbg_module_classes(const char *instr,
> [ ... ]
> >       case DD_CLASS_TYPE_LEVEL_NUM:
> >               /* input is bitpos, of highest verbosity to be enabled */
> >               if (inrep > map->length) {
> > -                     pr_warn("%s: level:%ld exceeds max:%d, clamping\n",
> > +                     pr_warn("%s: level:%u exceeds max:%d, clamping\n",
> >                               KP_NAME(kp), inrep, map->length);
> >                       inrep = map->length;
> >               }
> >               old_bits = CLASSMAP_BITMASK(*dcp->lvl);
> >               new_bits = CLASSMAP_BITMASK(inrep);
>
> [Severity: Medium]
> Will this silently prevent enablement of classes >= 32?
>
> CLASSMAP_BITMASK was changed to limit the generated mask to 32 bits, but
> map->length is not restricted to 32.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19