From: Dan Carpenter <dan.carpenter@oracle.com>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
Phillip Potter <phil@philpotter.co.uk>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Pavel Skripkin <paskripkin@gmail.com>,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
David Laight <david.Laight@aculab.com>,
Martin Kaiser <martin@kaiser.cx>
Subject: Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()
Date: Mon, 20 Sep 2021 22:01:33 +0300 [thread overview]
Message-ID: <20210920190133.GS2116@kadam> (raw)
In-Reply-To: <3187315.KoBY3qX4Pt@localhost.localdomain>
On Mon, Sep 20, 2021 at 06:17:36PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:
> > On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:
> > > On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> > > > On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > > > > Change the type of "data" from __le32 to __le16.
> > > > >
> > > >
> > > > You should note in the commit message that:
> > > >
> > > > The last two bytes of "data" are not initialized so the
> le32_to_cpu(data)
> > > > technically reads uninitialized data. This can likely be detected by
> > > > the KASan checker as reading uninitialized data. But because the bytes
> > > > are discarded in the end so this will not affect runtime.
> > > >
> > > > regards,
> > > > dan carpenter
> > > >
> > >
> > > Dear Dan,
> > >
> > > Thanks for your suggestion about this specific topic.
> > >
> > > We thought that, since "data" is in bitwise AND with 0xffff before being
> > > passed to the callee, it was enough to have reviewers know why we're
> doing
> > > that change of type with no further explanations. Actually it seems to be
> not
> > > enough to motivate that change.
> > >
> > > We will surely use the note you provided.
> > >
> > > However, since I'm not used to blindly follow suggestions (even if I
> trust
> > > your words with no doubts at all) without complete understanding of what
> I'm
> > > doing, I will need to understand what KASan is before copy-paste your
> note.
> >
> > Google is your friend!
>
> Yes, it is :)
>
> I think you were referring to the KernelMemorySanitizer (KMSan), a detector
> of uses of uninitialized memory (but it seems to not be upstream):
> https://github.com/google/kmsan
>
> Instead you wrote about the The Kernel Address Sanitizer (KASan) that seems
> to be a dynamic memory error detector designed to find out-of-bound and use-
> after-free bugs (this is upstream):
> https://www.kernel.org/doc/html/v5.0/dev-tools/kasan.html
>
> Can you please confirm?
That sounds probably right.
>
> Back to the code... uninitialised data is not a problem in the old code, it's
> just bad design. The new code cannot affect runtime, it's just better design.
It would be a problem for KMSan and the kbuild-bot will email you about
it. From your commit message "Change the type of "data" from __le32 to
__le16." it's not clear you understand why the kbuild-bot will email you
and why it's correct to do so.
regards,
dan carpenter
next prev parent reply other threads:[~2021-09-20 19:05 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-19 23:53 [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 01/19] staging: r8188eu: clean up symbols usbctrl_vendorreq() Fabio M. De Francesco
2021-09-20 11:46 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 02/19] staging: r8188eu: reorder declarations in usbctrl_vendorreq() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 03/19] staging: r8188eu: remove unnecessary test " Fabio M. De Francesco
2021-09-20 11:47 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 04/19] staging: r8188eu: reorder comments " Fabio M. De Francesco
2021-09-20 11:48 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 05/19] staging: r8188eu: remove unnedeed parentheses " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 06/19] staging: r8188eu: remove unnecessary space " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 07/19] staging: r8188eu: remove unnecessary comment " Fabio M. De Francesco
2021-09-20 11:48 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 08/19] staging: r8188eu: fix grammar mistake " Fabio M. De Francesco
2021-09-20 11:49 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 09/19] staging: r8188eu: remove unnecessary braces " Fabio M. De Francesco
2021-09-20 11:49 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 10/19] staging: r8188eu: rename symbols in rtw_read*() and rtw_write*() Fabio M. De Francesco
2021-09-20 11:50 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 11/19] staging: r8188eu: remove unnecessary casts from rtw_{read,write}*() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 12/19] staging: r8188eu: change the type of a variable in rtw_write16() Fabio M. De Francesco
2021-09-20 11:50 ` Greg Kroah-Hartman
2021-09-19 23:53 ` [PATCH v8 13/19] staging: r8188eu: remove an unneeded buffer from rtw_writeN() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 14/19] staging: r8188eu: remove an unnecessary bit AND " Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16() Fabio M. De Francesco
2021-09-20 11:51 ` Greg Kroah-Hartman
2021-09-20 11:56 ` Dan Carpenter
2021-09-20 13:03 ` Fabio M. De Francesco
2021-09-20 13:10 ` Dan Carpenter
2021-09-20 16:17 ` Fabio M. De Francesco
2021-09-20 19:01 ` Dan Carpenter [this message]
2021-09-20 19:54 ` Fabio M. De Francesco
2021-09-21 5:35 ` Dan Carpenter
2021-09-19 23:53 ` [PATCH v8 16/19] staging: r8188eu: call the new usb_read() from rtw_read{8,16,32}() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 17/19] staging: r8188eu: call the new usb_write() from rtw_write{8,16,32,N}() Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 18/19] staging: r8188eu: remove shared buffer for USB requests Fabio M. De Francesco
2021-09-19 23:53 ` [PATCH v8 19/19] staging: r8188eu: remove usb_vendor_req_mutex Fabio M. De Francesco
2021-09-20 11:55 ` [PATCH v8 00/19] staging: r8188eu: shorten and simplify calls chains Greg Kroah-Hartman
2021-09-20 13:44 ` Fabio M. De Francesco
2021-09-20 14:06 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210920190133.GS2116@kadam \
--to=dan.carpenter@oracle.com \
--cc=Larry.Finger@lwfinger.net \
--cc=david.Laight@aculab.com \
--cc=fmdefrancesco@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=martin@kaiser.cx \
--cc=paskripkin@gmail.com \
--cc=phil@philpotter.co.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox