Received-SPF: pass (google.com: domain of linux-kernel+bounces-128902-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d572be26-0217-4522-8cc4-b9c6a62e6f7c@embeddedor.com>
Date: Tue, 2 Apr 2024 18:49:34 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH][next] nfp: Avoid -Wflex-array-member-not-at-end warnings
To: Jakub Kicinski <kuba@kernel.org>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Louis Peens <louis.peens@corigine.com>,
 "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Paolo Abeni <pabeni@redhat.com>, oss-drivers@corigine.com,
 netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org
References: <ZgYWlkxdrrieDYIu@neat> <20240401212424.34a9a9cd@kernel.org>
Content-Language: en-US
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <20240401212424.34a9a9cd@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 01/04/24 22:24, Jakub Kicinski wrote:
> On Thu, 28 Mar 2024 19:17:10 -0600 Gustavo A. R. Silva wrote:
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
>> @@ -34,8 +34,11 @@ enum nfp_dumpspec_type {
>>   
>>   /* generic type plus length */
>>   struct nfp_dump_tl {
>> -	__be32 type;
>> -	__be32 length;	/* chunk length to follow, aligned to 8 bytes */
>> +	/* New members must be added within the struct_group() macro below. */
>> +	struct_group_tagged(nfp_dump_tl_hdr, hdr,
>> +		__be32 type;
>> +		__be32 length;	/* chunk length to follow, aligned to 8 bytes */
>> +	);
>>   	char data[];
>>   };
> 
> I counted 9 references to nfp_dump_tl->data.
> Better to add:
> 
> static void *nfp_dump_tl_data(struct nfp_dump_tl *spec)
> {
> 	return &spec[1];
> }

Unfortunately, that's out-of-bounds for the compiler, and well, basically
the reason why flex-array members were created in the first place.

I was looking into implementing two separate structs:

struct nfp_dump_tl_hdr {
         __be32 type;
         __be32 length;  /* chunk length to follow, aligned to 8 bytes */
};

struct nfp_dump_tl {
         __be32 type;
         __be32 length;  /* chunk length to follow, aligned to 8 bytes */
	char data[];
};

and at least for structs nfp_dumpspec_csr, nfp_dumpspec_rtsym, nfp_dump_csr, and
nfp_dump_rtsym it'd be a clean change (no need for container_of()), but not for
structs nfp_dumpspec_csr and nfp_dumpspec_rtsym because of some casts from
the flex struct:

nfp_add_tlv_size():
         case NFP_DUMPSPEC_TYPE_ME_CSR:
                 spec_csr = (struct nfp_dumpspec_csr *)tl;
                 if (!nfp_csr_spec_valid(spec_csr))
		...

         case NFP_DUMPSPEC_TYPE_INDIRECT_ME_CSR:
                 spec_csr = (struct nfp_dumpspec_csr *)tl;
                 if (!nfp_csr_spec_valid(spec_csr))
		...

	case NFP_DUMPSPEC_TYPE_RTSYM:
                 spec_rtsym = (struct nfp_dumpspec_rtsym *)tl;
                 err = nfp_dump_single_rtsym(pf, spec_rtsym, dump);

nfp_calc_rtsym_dump_sz():
         spec_rtsym = (struct nfp_dumpspec_rtsym *)spec;


At least for those two structs, it's probably more straightforward to use
struct_group_tagged() and container_of().

--
Gustavo