mbox series

[0/8] scsi: aacraid: Replace one-element arrays with flexible-array members

Message ID cover.1645513670.git.gustavoars@kernel.org (mailing list archive)
Headers show
Series scsi: aacraid: Replace one-element arrays with flexible-array members | expand

Message

Gustavo A. R. Silva Feb. 22, 2022, 7:28 a.m. UTC
This series aims to replace one-element arrays with flexible-array
members in multiple structures in drivers/scsi/aacraid/aacraid.h.

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

This helps with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().

These issues were found with the help of Coccinelle and audited and fixed,
manually.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays

Link: https://github.com/KSPP/linux/issues/79

Gustavo A. R. Silva (8):
  scsi: aacraid: Replace one-element array with flexible-array member
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmap
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmap
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmap64
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmap64
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct sgmapraw
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct user_sgmapraw
  scsi: aacraid: Replace one-element array with flexible-array member in
    struct aac_aifcmd

 drivers/scsi/aacraid/aachba.c   | 76 +++++++++++----------------------
 drivers/scsi/aacraid/aacraid.h  | 16 +++----
 drivers/scsi/aacraid/commctrl.c |  5 +--
 drivers/scsi/aacraid/comminit.c |  3 +-
 4 files changed, 37 insertions(+), 63 deletions(-)

Comments

Gustavo A. R. Silva March 10, 2022, 4:03 a.m. UTC | #1
Hi all,

Friendly ping: who can review or comment on this series, please?

Thanks
--
Gustavo

On Tue, Feb 22, 2022 at 01:28:12AM -0600, Gustavo A. R. Silva wrote:
> This series aims to replace one-element arrays with flexible-array
> members in multiple structures in drivers/scsi/aacraid/aacraid.h.
> 
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> This helps with the ongoing efforts to globally enable -Warray-bounds
> and get us closer to being able to tighten the FORTIFY_SOURCE routines
> on memcpy().
> 
> These issues were found with the help of Coccinelle and audited and fixed,
> manually.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> 
> Gustavo A. R. Silva (8):
>   scsi: aacraid: Replace one-element array with flexible-array member
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmap
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmap
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmap64
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmap64
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct sgmapraw
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct user_sgmapraw
>   scsi: aacraid: Replace one-element array with flexible-array member in
>     struct aac_aifcmd
> 
>  drivers/scsi/aacraid/aachba.c   | 76 +++++++++++----------------------
>  drivers/scsi/aacraid/aacraid.h  | 16 +++----
>  drivers/scsi/aacraid/commctrl.c |  5 +--
>  drivers/scsi/aacraid/comminit.c |  3 +-
>  4 files changed, 37 insertions(+), 63 deletions(-)
> 
> -- 
> 2.27.0
>
Martin K. Petersen March 15, 2022, 4:02 a.m. UTC | #2
Gustavo,

> Friendly ping: who can review or comment on this series, please?

I'm afraid I don't have any hardware to test it on and the generated
output differs substantially from the original code.
Gustavo A. R. Silva March 15, 2022, 4:22 a.m. UTC | #3
On Tue, Mar 15, 2022 at 12:02:13AM -0400, Martin K. Petersen wrote:
> 
> Gustavo,
> 
> > Friendly ping: who can review or comment on this series, please?
> 
> I'm afraid I don't have any hardware to test it on and the generated
> output differs substantially from the original code.

Yeah; this series requires careful review from the people that
knows the code better. 

It took me a day of work to go through all the places that needed
to be changed due to the flexible array transformation. However,
due to the kind of changes, it'd be great to have a second opinion
or at least someone that could take a look at the changes.

Thanks
--
Gustavo
Kees Cook June 23, 2022, 4:26 p.m. UTC | #4
On Mon, Mar 14, 2022 at 11:22:23PM -0500, Gustavo A. R. Silva wrote:
> On Tue, Mar 15, 2022 at 12:02:13AM -0400, Martin K. Petersen wrote:
> > 
> > Gustavo,
> > 
> > > Friendly ping: who can review or comment on this series, please?
> > 
> > I'm afraid I don't have any hardware to test it on and the generated
> > output differs substantially from the original code.
> 
> Yeah; this series requires careful review from the people that
> knows the code better. 
> 
> It took me a day of work to go through all the places that needed
> to be changed due to the flexible array transformation. However,
> due to the kind of changes, it'd be great to have a second opinion
> or at least someone that could take a look at the changes.

If the int/size_t changes are separated from the array size change, it's
easier to see the array size change is a binary no-op. (i.e. diffoscope
shows no executable changes.)

I'd recommend splitting the int/size_t changes from the array size
changes.