From: Kees Cook <keescook@chromium.org>
To: James Seo <james@equiv.tech>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>,
Sreekanth Reddy <sreekanth.reddy@broadcom.com>,
Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
MPT-FusionLinux.pdl@broadcom.com, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/6] scsi: mpt3sas: Use flexible arrays when less obviously possible
Date: Fri, 28 Jul 2023 15:26:57 -0700 [thread overview]
Message-ID: <202307281508.32604C856@keescook> (raw)
In-Reply-To: <20230725161331.27481-3-james@equiv.tech>
On Tue, Jul 25, 2023 at 09:13:27AM -0700, James Seo wrote:
> These old-style 1-length variable arrays can be directly converted
> into C99 flexible array members without any further source changes
> and without any meaningful binary changes. All uses of the affected
> structs were investigated, and the existing code somehow manages to
> weather the reduced sizeof() the affected structs in every case.
Doing build comparisons here, I see a lot of binary changes. They may
be, as you say, harmless, and since you've actually got hardware then
this is a good verification of the changes, but I do wonder if this
needs more detailed commit log (or split up patches).
However, the problem I see is that this code was already doing weird
stuff with structs that appear to not have been using flex arrays
actually. With "pahole" I can see struct MPT3SAS_ADAPTER changes:
- Mpi2IOUnitPage8_t iounit_pg8; /* 3668 40 */
- Mpi2IOCPage1_t ioc_pg1_copy; /* 3708 24 */
+ Mpi2IOUnitPage8_t iounit_pg8; /* 3668 16 */
+ Mpi2IOCPage1_t ioc_pg1_copy; /* 3684 24 */
struct _MPI2_CONFIG_PAGE_IO_UNIT_8 (Mpi2IOUnitPage8_t) is in the
_middle_ of struct MPT3SAS_ADAPTER.... :|
In the earlier attempts at this conversion, it seemed that some of these
are actually fixed-size:
https://lore.kernel.org/lkml/20210202235118.GA314410@embeddedor/
I think this patch needs to be broken up into per-struct changes, so
they can be reviewed individually.
-Kees
--
Kees Cook
next prev parent reply other threads:[~2023-07-28 22:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-25 16:13 [PATCH 0/6] scsi: mpt3sas: Use flexible arrays and do a few cleanups James Seo
2023-07-25 16:13 ` [PATCH 1/6] scsi: mpt3sas: Use flexible arrays when obviously possible James Seo
2023-07-28 22:01 ` Kees Cook
2023-07-25 16:13 ` [PATCH 2/6] scsi: mpt3sas: Use flexible arrays when less " James Seo
2023-07-28 22:26 ` Kees Cook [this message]
2023-07-29 8:09 ` James Seo
2023-07-25 16:13 ` [PATCH 3/6] scsi: mpt3sas: Use struct_size() for struct size calculations James Seo
2023-07-25 16:13 ` [PATCH 4/6] scsi: mpt3sas: Fix an outdated comment James Seo
2023-07-28 22:27 ` Kees Cook
2023-07-25 16:13 ` [PATCH 5/6] scsi: mpt3sas: Fix typo of "TRIGGER" James Seo
2023-07-28 22:27 ` Kees Cook
2023-07-25 16:13 ` [PATCH 6/6] scsi: mpt3sas: Replace a dynamic allocation with a local variable James Seo
2023-07-28 22:29 ` Kees Cook
2023-07-29 8:35 ` James Seo
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=202307281508.32604C856@keescook \
--to=keescook@chromium.org \
--cc=MPT-FusionLinux.pdl@broadcom.com \
--cc=gustavoars@kernel.org \
--cc=james@equiv.tech \
--cc=jejb@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=sathya.prakash@broadcom.com \
--cc=sreekanth.reddy@broadcom.com \
--cc=suganath-prabu.subramani@broadcom.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.