Received-SPF: pass (google.com: domain of linux-kernel+bounces-93399-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <bd21f7dc-9f89-40ee-895e-601c80165225@intel.com>
Date: Wed, 6 Mar 2024 08:06:29 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] overflow: Change DEFINE_FLEX to take __counted_by member
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>, Kees Cook
	<keescook@chromium.org>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen
	<anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	<intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
	<linux-hardening@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>,
	<linux-kernel@vger.kernel.org>
References: <20240306010746.work.678-kees@kernel.org>
 <9c2990f0-7407-49c6-9e3a-b92de82ea437@embeddedor.com>
Content-Language: en-US
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <9c2990f0-7407-49c6-9e3a-b92de82ea437@embeddedor.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NnNocjRPOW9PU2E5WnArZDNucUxwcVd6SFNvVXhvNkwyTFBkdTVyVkVvVGRU?=
 =?utf-8?B?enZOaEl6di9id09WWnQvdkVVWVo5R0V3dGw3dWdNOSt5eFQvQms3cmRlcmtY?=
 =?utf-8?B?aE95ZytVWmY1ejlucHZHcUV6K0NyWjFPQzBucnBudGJ5MzV3bGx1VjJmN3BP?=
 =?utf-8?B?a2FGb0oxU21kcSsyTi85cnB4NE95b3VOZWdEdjNSQTMvdlMwY0N2VjRqWng4?=
 =?utf-8?B?REpJRFJNT3RuMjZ1Ty9pTTQ1aEZkUENQL2JLZVBqNWtUZnpGVm1PM2lYWUtz?=
 =?utf-8?B?U0tWSDZHNXNSUEZlcHY4NFhabXZWZDlTYjlteDFCRDUrdjRoWTNVNExha081?=
 =?utf-8?B?ekZScVVxOEVMMEZuUlVNMmlsbFdodWFIRFZaSURKSFl1QUlGVHlNTmFlRGNR?=
 =?utf-8?B?eEJyZHdoT2FSbVk0NXduS1ExYVhBTi9Qb0tXQ1Z3Q1NBS2t6czZvOVZXVW5Z?=
 =?utf-8?B?YWFUNXZvRUNCUTJOSnlvRzRiVzArREMwNnZHbGY2MnNRT2dtWWtPd3R2Vkdi?=
 =?utf-8?B?TkNLZmZjWDNzMlBORmU0UEhxcUFRU3hXZVQyYUI0bXFPdnREQnk2cmhqV0J6?=
 =?utf-8?B?N3U1T1dEN0NtdUFrc1JyRUxZbHhFdjcvWHNlTGF5VGJxSGlKWWZHQnVtR0pJ?=
 =?utf-8?B?SkJPbnNSM0dJU0NCNHJsZDBxbERGSFpGYkVEalVBUzc1RC8xZk84a0h5YklW?=
 =?utf-8?B?NUxiL1BCZnA4THBmR2tYbW5EaWFNT3FqanBJZDFWNW5PMzdUVUl4UTlsd3py?=
 =?utf-8?B?bCtrbG1CcktaTDlSQ241TFU5MDdURXNFQVFFWWtubkxZN0pBNDl5cHJMcUwv?=
 =?utf-8?B?bko5ckREVEFkdHAzMmFxS08xVldlaDR3RXd4VnpRUVRTZXJuVzF4NnZKWXdi?=
 =?utf-8?B?aEdzQ2cvb1VaZEpCMk5PWDVoNEREdFhrbDVSc041ZFUzcXYwd1pOcWlSNFZM?=
 =?utf-8?B?TVdqUnl5ZURkS0ZrMVI2dklMaGhyeHRQZTJibFZBNkdJL09vOFZVTWhaQ29z?=
 =?utf-8?B?SDBkaUg3VVIzUi90KzQ4bThVRjZROWVDQW1kNHFCK3h6V2hPeWlxQ0lPblA0?=
 =?utf-8?B?RGxSSk9GajVVak5RN2hCTk5XUElkN2NxTGZQRnovRXdFZWl4Y1o2Qzk1RGJi?=
 =?utf-8?B?VkZvdU90UUNqVVZ3NmdPSjlnOVFYekpqbWZ2WjZYQmtxQml4d0VoaURrbE41?=
 =?utf-8?B?dnlVV2pESWdWZ1ZnTFppd0xjM3ZxL21tVWNJU1pSL0x4NCt2TWFaSjFLSjVx?=
 =?utf-8?B?WEVmSzJqTFJreGtXVjBEZTZsNEpwb0xpOVBnK285dktrN0FjU000bXk5V2hC?=
 =?utf-8?B?TStkTGhhTkgzQnh4SXZ2SzNYSWlaTmR0NWRHdGZRQUlOOUdpcVVFWmxJbmRV?=
 =?utf-8?B?UE1qOUw2OHJEdWJaa0t6Wms3ajVzMmZpeUZ6QWFmM3ZjMVhLSU8xWDFVUUlo?=
 =?utf-8?B?M3Yya3dFekJxMDdLZHI4czk2WEYwNHk1cWp4eDRSWURFdnNENTVaeWFtekJw?=
 =?utf-8?B?RDFTbGgwTnJ2emRMMzNySFc3aFFkYXVrZ0s1ZHJudnBDZllTa3dxdENQTEIz?=
 =?utf-8?B?M3hpS1BqWGlNWjlEb3BpbGlncytpcHozNEpwTE9xckl2UWRKamhPRHBNZVJS?=
 =?utf-8?B?QUVURFZXTUp0SGJxMGQ5UWZscktyV2FkdlUxZWpLN1dNM2VtK3cyWStPQS9M?=
 =?utf-8?B?S3BGcUZrQzE5d2owQWZMZ0VPSEJudW1JeFl2cVNNdElZMklGeTJ6RHczN2k1?=
 =?utf-8?B?TlAycU0rZjNSVS83LzFvM0gzRW9lVTJxVFZVZlVHb210WW5JeEhNN1ljQy8r?=
 =?utf-8?B?RC9mRHFhSHMyZVdSZFQyUnFUSFhDbXlRYkc2dFY5V2dsS21NVTRia1RCL01l?=
 =?utf-8?B?eVEyRklFdlZWUGdUc1ZVTzJjMzB5ZnF5cmJUWHMxUEdtU3lpUGFHY2ZDUFBO?=
 =?utf-8?B?VHV0dGRBM0s0NGZtMjJWQU5jajVwdk16b0hobUZyRnB1UHBpU2FxMXdHeDNy?=
 =?utf-8?B?UjBqeUI5d0g3bVhqSUVPZHhzWjR4UURBMzQxbXdER0RTZEhZTkc0aDg0dUlO?=
 =?utf-8?B?ZGRBcEZiRlFKb09LNjNBYU9jcFEvdVlkZXJkWFo5RTFlN0ZIQjNNWUF0WVRI?=
 =?utf-8?B?TURodW1sam9xa2hnNmFKSk5yRkI1QzJnanVnMWNuVjJsMG85TDRrN2lPeEgw?=
 =?utf-8?B?cVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0f7ab7f6-7ee5-4819-eac1-08dc3dabf754
X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8102.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 07:06:37.9234
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z79nPhBMTQGN2Vw92b6p2SPoEax/SGyt5PfcdxqlGrDoD0fUe2aghIq3+y5QzyQ9XCHw9q3R3O5+XTSnzaNeKArqxc7g52OCa+NQIdQvsoQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4589
X-OriginatorOrg: intel.com

On 3/6/24 04:25, Gustavo A. R. Silva wrote:
> 
> 
> On 05/03/24 19:07, Kees Cook wrote:
>> The norm should be flexible array structures with __counted_by
>> annotations, so DEFINE_FLEX() is updated to expect that. Rename
>> the non-annotated version to DEFINE_RAW_FLEX(), and update the few
>> existing users. Additionally add self-tests to validate syntax and
>> size calculations.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
> 
> [..]

Just a note that ice changes are purely mechanical, so this seems ok
to go via linux-hardening tree. And changes per-se are fine too :)

> 
>> +/**
>> + * DEFINE_FLEX() - Define an on-stack instance of structure with a 
>> trailing
>> + * flexible array member.
>> + *
>> + * @TYPE: structure type name, including "struct" keyword.
>> + * @NAME: Name for a variable to define.
>> + * @COUNTER: Name of the __counted_by member.
>> + * @MEMBER: Name of the array member.
>> + * @COUNT: Number of elements in the array; must be compile-time const.
>> + *
>> + * Define a zeroed, on-stack, instance of @TYPE structure with a 
>> trailing
>> + * flexible array member.
>> + * Use __struct_size(@NAME) to get compile-time size of it afterwards.
>> + */
>> +#define DEFINE_FLEX(TYPE, NAME, COUNTER, MEMBER, COUNT)    \
> 
> Probably, swapping COUNTER and MEMBER is better?

right now we have usage scenario (from Kunits):
	DEFINE_FLEX(struct foo, eight, counter, array, 8);

> 
>      DEFINE_FLEX(TYPE, NAME, MEMBER, COUNTER, COUNT)

usage would become:
	DEFINE_FLEX(struct foo, eight, array, counter, 8);

which reads a bit better indeed, with the added benefit that we
go from broader to more specific:
whole struct -> array -> array size variable -> given array size

so +1 from me for the params swap

> 
> Thanks
> -- 
> Gustavo