linux-kernel - Re: [PATCH v2] EDAC/i10nm: shift exponent is negative

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJB-X+X+09-m57JcZcb-_9dKUG3CtAbLXxGTEg7R0bB8pyJx9Q@mail.gmail.com>
Date:   Tue, 4 Jul 2023 10:20:20 +0800
From:   Koba Ko <koba.ko@...onical.com>
To:     "Luck, Tony" <tony.luck@...el.com>
Cc:     Markus Elfring <Markus.Elfring@....de>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        James Morse <james.morse@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Richter <rric@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] EDAC/i10nm: shift exponent is negative

On Tue, Jul 4, 2023 at 5:51 AM Luck, Tony <tony.luck@...el.com> wrote:
>
> > > UBSAN complains this error
> > > ~~~
> > >  UBSAN: shift-out-of-bounds in drivers/edac/skx_common.c:369:16
> > …
> > > ~~~
> > >
> > > when get rows, cols and ranks, the returned error value doesn't be
> > > handled.
> > >
> > > check the return value is EINVAL, if yes, directly return 0.
> > …
> >
> > * Please improve this change description further.
>
> To be specific. Initially you reported this because of the UBSAN error
> report. But, after community discussion you now know that the problem
> is that one or more of the rows/cols/ranks has a value that the EDAC driver
> doesn't expect and probably can handle.
>
> So, in V2, the commit message should start with the information these
> values are out of range and mention this was discovered when UBSAN
> put out a warning about a negative shift. No need to include the whole
> of the UBSAN stack trace.
>
> Then describe the two fixes that this patch includes. One is to change the
> edac debug message into a console error message to enable further
> debug of this issue. The other is to skip the unrecognized DIMM.
>
> > * How do you think about to add the tag “Fixes”?
>
> This is a good idea.  Use git blame, or dig into the GIT history to
> find the commit where this code was introduced (hint .. git blame
> says:
> 88a242c98740 ("EDAC, skx_common: Separate common code out from skx_edac")
> but that obviously just refactored code, so you should dig back more into
> the history.
There are two parts,
1. @get_dimm_attr, edac_dbg was added since e235dd43d8b0f0
2. get num of ranks, rows and cols, 4ec656bdf43a13

Should I add all of them prefixes with "Fixes"?

>
> > > V2: make error-print explicitly
> > > ---
> >
> > Would you like to avoid a misplaced marker line here?
> >
> > See also:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n686
>
> That's an excellent resource.
>
> -Tony