Re: [PATCH][next] scsi: pm80xx: Avoid -Wflex-array-member-not-at-end warning

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 9/4/25 14:39, James Bottomley wrote:
> On Thu, 2025-09-04 at 07:52 +0100, John Garry wrote:
>> On 03/09/2025 19:44, Gustavo A. R. Silva wrote:
>>> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h
>>> b/drivers/scsi/pm8001/pm8001_hwi.h
>>> index fc2127dcb58d..7dc7870a8f86 100644
>>> --- a/drivers/scsi/pm8001/pm8001_hwi.h
>>> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
>>> @@ -339,8 +339,10 @@ struct ssp_completion_resp {
>>>    	__le32	status;
>>>    	__le32	param;
>>>    	__le32	ssptag_rescv_rescpad;
>>> -	struct ssp_response_iu  ssp_resp_iu;
>>>    	__le32	residual_count;
>>> +
>>> +	/* Must be last --ends in a flexible-array member. */
>>> +	struct ssp_response_iu  ssp_resp_iu;
>>
>> this is a HW structure, right? I did not think that it is ok to
>> simply re-order them...
> 
> Agreed, this is a standards defined information unit corresponding to
> an on the wire data structure.  The patch is clearly wrong.
> 
> That being said, the three things the flexible member can contain are
> no data, response data or sense data.  None of them has a residual
> count at the beginning and, indeed, this field is never referred to in
> the driver, so it looks like it can simply be deleted to fix the
> warning.

I see. I just submitted v2. Thanks!

> 
> That being said, this pattern of adding fields after flexible members
> to represent data that's common to all content types of the union is
> not unknown in SCSI so if you want to enable this warning, what are we
> supposed to do when we encounter a genuine use case?

It depends on the situation. Some people prefer to have a separate struct
with only the header part of the flexible structure --this is excluding
the flexible-array member (FAM), and then use an object of that header
type embedded anywhere in any other struct. This of course (sometimes)
implies having to do many other changes to accommodate the rest of the
code accordingly, as in this[1] case.

To address the situation described above without necessarily having to
create a separate header struct and change a lot of code, the
struct_group() helper can be used [2].

In other cases, when for some reason the FAM has to be accessed through
a composite struct, both struct_group() and container_of() (this to
retrieve a pointer to the flexible structure and then access the FAM)
can be used [3].

We have the new TRAILING_OVERLAP() helper that in many cases can be
superior to the struct_group()/container_of() approach. For instance
this[4] could've been fixed with the following shorter and simpler
patch:

  struct nfsacl_simple_acl {
-       struct posix_acl acl;
-       struct posix_acl_entry ace[4];
+       TRAILING_OVERLAP(struct posix_acl, acl, a_entries,
+               struct posix_acl_entry ace[4];
+       );
  };

However, at the time we didn't have TRAILING_OVERLAP(). Also, this new
macro is helping us to detect alignment issues and correct them [5].
These[6][7] are a couple more example of when and how to use this helper.

We also have the DEFINE_FLEX()/DEFINE_RAW_FLEX() helpers [8][9].

So, again, we can do different things depending on the situation and
maintainer's preferences.

Ideally, FAMs-in-the-middle should be avoided, because it's so easy
for them to open the door to memory corruption bugs[10] (to mention
some).

Thanks
-Gustavo

[1] https://git.kernel.org/linus/d2af710d6d50
[2] https://git.kernel.org/linus/c54979a3abc4
[3] https://git.kernel.org/linus/a7e8997ae18c
[4] https://git.kernel.org/linus/dfd500d89545
[5] https://lore.kernel.org/linux-hardening/aLiYrQGdGmaDTtLF@kspp/
[6] https://git.kernel.org/linus/5e54510a9389
[7] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=8abbbbb588f1
[8] https://git.kernel.org/linus/1d717123bb1a
[9] https://git.kernel.org/linus/34116ec67cc1
[10a] https://git.kernel.org/linus/eea03d18af9c
[10b] https://git.kernel.org/linus/6e4bf018bb04
[10c] https://git.kernel.org/linus/cf44e745048d
[10d] https://git.kernel.org/linus/d761bb01c85b