Received-SPF: pass (google.com: domain of linux-kernel+bounces-168770-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 4 May 2024 19:21:48 +0200
From: Erick Archer <erick.archer@outlook.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Kees Cook <keescook@chromium.org>,
	Erick Archer <erick.archer@outlook.com>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	"Liang, Kan" <kan.liang@linux.intel.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Justin Stitt <justinstitt@google.com>,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] perf/ring_buffer: Prefer struct_size over open coded
 arithmetic
Message-ID:
 <AS8PR02MB7237E1240937477D0D6F57AB8B1E2@AS8PR02MB7237.eurprd02.prod.outlook.com>
References: <AS8PR02MB72372AB065EA8340D960CCC48B1B2@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <20240430091504.GC40213@noisy.programming.kicks-ass.net>
 <202405011317.AF41B94B@keescook>
 <20240502091837.GA30852@noisy.programming.kicks-ass.net>
 <202405021552.5C000EA@keescook>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202405021552.5C000EA@keescook>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: AS8PR02MB7237:EE_|DB9PR02MB6716:EE_
X-MS-Office365-Filtering-Correlation-Id: bd7fba74-f48d-47e6-f17c-08dc6c5eaf9e
X-Microsoft-Antispam:
	BCL:0;ARA:14566002|461199019|440099019|3412199016|56899024|1710799017;
X-Microsoft-Antispam-Message-Info:
	Mq/Q6UKXU7XQqC0r/7h+2R9/+ZlSW0sEdoo5x54+MMA0h0a7JxD06r1W8xZH9pGuYnZCLb7TfTc9E51FZoMiOhZ2Hl5vhIo+wIzTy61g8CRbNkMBu5y1c9TvdkkosFl7441ArUtC08hsq2+xi+ORhVe5BSSEu6sLomm99iW+FTorGgIHYVz9FeWPawofP1T2y6MuVhETkS5+4DmsSImP/kty1T8PwK3ofuad+djhw/0uUTaVyjW+nqL1HBru3cpnpBVrc2GDnOCqXoUZ5nrtbe4+m4SubwFRWoxrBIGMOVsSwzqXWS8KcAWIzyKe8gVYvFxiaArOooRRtUoSu53WYdrBhgtOydZQRqZAim//mBuwlUpL4Q+UawVVQe7FVhSxRZWYrOE31FwCHPhNcJJVEnUpUQJzD4wOhdndI/XO44rcQLCq1aeNyMl8A9CWy1KLLYMH72z9WtwxZI4qDiLRNzimwgIG6cvm+CfxmcNj25+nwNnWKTubxfhyyQjrdvOkX+JeZrFikTHzleXAiWYjoqnxFYNnHKnK12q6X6ls8EQXr0z2KQPVkDAu122+DE72dNLV6iw4VicN8Jql55nq733g1Gd0YsX3/OXQ4ZrIWixZb6uZh6ZOi4G88xLzsrADL9uRK4teqJbOPEC+hN8/XNpz2qcPHa0coZaApamld7s=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?qQfS5W3zWnt2WaUdUiuYYjk8pjeEyQjc6hG+9c40mdD9vYkMEwOg3acQP2QZ?=
 =?us-ascii?Q?d5SY1jjrE9hpMK8lWAVsAmhR404mX9DEqKQ9+Q8D06Rq5jKmGwX83iasHz4d?=
 =?us-ascii?Q?CzVmsLQJErX87w1mRjvhRjUFoRRvivB/jn7TGI8HrtZVox744fxXLvLeNIp3?=
 =?us-ascii?Q?fXfT2NJxi1JhElFm/GMJmJmFHFGfSDF0Ou3F3Vww7Oj9foCO/bvazftKfamq?=
 =?us-ascii?Q?0ir4imnWGfDXuYvGsKL8f4rDuM57qoUdtW/0N/nxYOXGkjIu95mz5YiJQgke?=
 =?us-ascii?Q?VpDN3Lzuz2cHcD5EVn+2DmZqH1+cEFDfzY1KOsj1O4XPhxFe+E6UeAdoXaR+?=
 =?us-ascii?Q?05bbWk04y7jSY/J3hfg9YGqlVhJaObYIT7Iv59sQKRfexvnEFQ8w2edomuxr?=
 =?us-ascii?Q?aIDrNUWhNFFDycMh3Bt0gTcrMPG08N2evp54KYvI/KTgoeehvdckovuGlKyU?=
 =?us-ascii?Q?+DvP6JLPIXY0RxKz3j57vkZ3LpDqRTuDqEdQweQThmIE7ZCnbP1GDNzSz/WW?=
 =?us-ascii?Q?2Y0/NQhqVu3HY2OfBxzkYP1JM3sF9ns7//K44EhOSIzy0o8+uJs0yi0b3r86?=
 =?us-ascii?Q?Qb4mZzliXeV1JYfk3xYOeiridBbMKqnKzc2ECYbLlgtK4zypsWK9sxuf4IRG?=
 =?us-ascii?Q?+XrVvEu9FPJQ7vxYlv6yEDHfckLyeXR0jl0547ktBzCzR7/rW9q51UsyvFdw?=
 =?us-ascii?Q?UJLkasHMGuEFAJQ8mbXuxbD+uOyUqGms63hd0+iOv0a1mbi/T2oUF3cTXK7+?=
 =?us-ascii?Q?mOhEpjXM/zRKJuA9MVeDCAAj/HJWvotV6coFpVsp4Ywy/YtO76IT8DtyXd4Y?=
 =?us-ascii?Q?TD8uEBbj3ze46N9Ab80p+oDVsi2gRXU3pPrd5/CJI9gOdB5wTKanauACQDHg?=
 =?us-ascii?Q?vJmMM2P3K8JmOjTWfi9BiM2jfJxf8rXlRqAeOo1SgioSp/wEjJY7ISOCGlTh?=
 =?us-ascii?Q?6/9laiTyZR7AQD9Nwaucw3KNO3OQ6AgC6DyEDHfmuyzES6absxnmeRUKNwvr?=
 =?us-ascii?Q?vSzHd8aIxkc3SCGkHGDDx7wYbqNu8SpkHtWG4AsFijGHxJJQfHCIlDzTnEEJ?=
 =?us-ascii?Q?+WuERPTKITQCgfMZtTVQlKD9xuPiEnFtV5f7ac0Exw5JM/yW5ouIZvxtcorJ?=
 =?us-ascii?Q?ifxmpEg+I8flf6qeVQ9YxHqZAW3k8PHpLOnUoWuFlvNfwIWZPKlAOZM21yNz?=
 =?us-ascii?Q?OU0g27JJ0qTqMjVslk4avA/aaGCq31wvqySoWnKQMT3j/qJx18WwsIvnvIVu?=
 =?us-ascii?Q?wWCBJoCIOWpbg6ygfcmm?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bd7fba74-f48d-47e6-f17c-08dc6c5eaf9e
X-MS-Exchange-CrossTenant-AuthSource: AS8PR02MB7237.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2024 17:21:51.1248
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR02MB6716

On Thu, May 02, 2024 at 03:55:36PM -0700, Kees Cook wrote:
> On Thu, May 02, 2024 at 11:18:37AM +0200, Peter Zijlstra wrote:
> > On Wed, May 01, 2024 at 01:21:42PM -0700, Kees Cook wrote:
> > > On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote:
> > > > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote:
> > > > > This is an effort to get rid of all multiplications from allocation
> > > > > functions in order to prevent integer overflows [1][2].
> > > > 
> > > > So personally I detest struct_size() because I can never remember wtf it
> > > > does, whereas the code it replaces is simple and straight forward :/
> > > 
> > > Sure, new APIs can involved a learning curve. If we can all handle
> > > container_of(), we can deal with struct_size(). :)
> > 
> > containre_of() is actually *much* shorter than typing it all out. Which
> > is a benefit.
> > 
> > struct_size() not so much. That's just obfuscation for obfuscation's
> > sake.

I do not agree with this.
> 
> It's really not -- it's making sure that the calculation is semantically
> sane: all the right things are being used for the struct size calculation
> and things can't "drift", if types change, flex array changes, etc. It's
> both a code robustness improvement and a wrap-around stopping improvement.
> 

Also, in the "Deprecated Interfaces, Language Features, Attributes, and
Conventions" [1] it says verbatim:

   Another common case to avoid is calculating the size of a structure
   with a trailing array of others structures, as in:

   header = kzalloc(sizeof(*header) + count * sizeof(*header->item),
                    GFP_KERNEL);

   Instead, use the helper:

   header = kzalloc(struct_size(header, item, count), GFP_KERNEL);

Therefore, if there is a convention to follow, we should not make an
exception. Moreover, struct_size is widely used in the kernel and
widely accepted. Also makes the code safer.

So, I will send a new patch with the changes Kees proposed and I
hope that it will be the first step in the adoption of struct_size
in the perf and sched subsystems ;)

Regards,
Erick

[1] https://docs.kernel.org/process/deprecated.html

> -- 
> Kees Cook