Received-SPF: pass (google.com: domain of linux-kernel+bounces-54518-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1ee4bd0f-2a70-4696-bad3-782b5c0887f7@prevas.dk>
Date: Tue, 6 Feb 2024 09:42:26 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and
 mul_wrap()
Content-Language: en-US, da
To: Kees Cook <kees@kernel.org>, Eric Biggers <ebiggers@kernel.org>,
 Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>, Marco Elver
 <elver@google.com>, linux-kernel@vger.kernel.org
References: <20240205090854.make.507-kees@kernel.org>
 <20240205091233.1357377-2-keescook@chromium.org>
 <20240205202145.GB2220@sol.localdomain> <202402051443.A813E4D@keescook>
 <20240205231712.GC2220@sol.localdomain>
 <51025C22-0FEF-4047-BC36-EA7ED7A3BD62@kernel.org>
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
In-Reply-To: <51025C22-0FEF-4047-BC36-EA7ED7A3BD62@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Y3cyWk9SdkZMOEdYOXp0cHVKbVFTeFBpcW9qSUNRTDIrNGZkUDd4b2NSQjU3?=
 =?utf-8?B?RzZwTzA4OFBuUDI3M2dPa0ZDV2l3ejUvKzRpZjNGbGRTRGlRSEFpSDJCckxT?=
 =?utf-8?B?Nk5ocmZONkgwSjhwWjJGZ0dEd0x2aThaMHVLZDBQZlYxUlZaY3RpYXNIWjBy?=
 =?utf-8?B?ZXBIMlMwQ2FRTUZQV01XMmQ5UDNEd1owc1FKUkI0blJ6SWNzZWNPcWdqU2lI?=
 =?utf-8?B?bVUwOTNGazZqcUUxYUZ3Z1hpV01UWjNCc1pRZnA3OCtSMU95SE11V08xSVpH?=
 =?utf-8?B?dkY2VWZzZDEzdERIL3pLY1ljZ2E3SkxjbUVtL2tPSzlESHUzcTBjTEsvdHpx?=
 =?utf-8?B?aU5UNHFpY3NOV0c4NXJQZC9nK0Z2aUJ6clkxMi9IYms4Q1YxRStlZzRzMzJC?=
 =?utf-8?B?V1ovU05lYlBZMTZXRjlnbGxWaTk0S1RvQlVTNnlDWDVaS244R01Lcjh1R2xq?=
 =?utf-8?B?anU3ZklheTUraHQwOXN0SUR0R2toWG5GWEExRXVUUVVWSkVEZ0VzRmVnNmM0?=
 =?utf-8?B?ODlCMGlkS3ZLemJZRzRFbFdYSG1wTlA5UHB3UTBva1o2SHZkUzFVUjZabjhR?=
 =?utf-8?B?QVRiczhrWlRjZTJEUXh4bThEcUxhTzRHM080eTlBU0dCYnRuaXVLcVJWTWRz?=
 =?utf-8?B?UytpSXYybDlEWitORU45NHlhVytRaDU2Kzg5cTdDcWdEa0hCMnFOdDhtK01x?=
 =?utf-8?B?MExpS3J3eWkzYnBrOTdrWkRMYXRIbHlJRXZTbUI1VmtTWEIwSHhIWVRhV0Nj?=
 =?utf-8?B?Wi9sZFhvYUlEV2xrSk5nNkJjOHRKcEwyOTlQNVJEKzVHWjNwb2xpUWY3WEtm?=
 =?utf-8?B?ZUxSZVR4cHY2Q1BpYUtzZG03bitMaldscDNvVEtFMjJJMmIwNHM2MGJiOHYv?=
 =?utf-8?B?aE1KbWtqM1gwT0lyVzJlcWFHR3U4a1huVWh2ajZXQTNrdGgzMDlld3hSTnVy?=
 =?utf-8?B?OHB6QVJuTXZZUEdjeXB6QnczcEtza2g0RzZ3cjdqZnFmWHd0Nm5zamdsTmJw?=
 =?utf-8?B?UlVTRHdwa21xUjk0WFRpdVlvaHhCZDBtaUVHQkR5UFNnZHpRYlRRbldtRFYv?=
 =?utf-8?B?ZDdWNjZwT3VHNy9HRTRKbE1xY2wvTmg5U3FnNk95L3dPalU5Nk5WSUxkcHlT?=
 =?utf-8?B?dkVaM08xMTZyQzNMMG50Q2Z2QThGbWVsSStkWDFjOVZwM3d1V0YySmNSQUpO?=
 =?utf-8?B?NERwcVVpZmwxK0VvaXlpTGtyQytXa3dzYVdjYytFMjFEcEdldmUwNmY4ckpm?=
 =?utf-8?B?Z1FwalpsYURlNTZabndmMmt3UVB0emR2Z2lKR0JtUHFZOVBLd3p3L0NXNmhC?=
 =?utf-8?B?eElYUFdzdWl3QTZEbkZUNXRJL2pUY0hzRldiOVdKSmUzTmM0S0kwT04vOEFW?=
 =?utf-8?B?U1lqRjNGSC9DMzhFMk1NNHNRQWZiZmw2YnNJTi96MFlYdFJoRlBBeTRtUFlj?=
 =?utf-8?B?djk2aFZIWi9TZ0o2c2UzT2FTbjlxc05SdG9mMVdldC81blhkWkxRNVJJRjV4?=
 =?utf-8?B?aTNJeXFyUGovMVJsNlNYaHoxa0o4TWtnb1Azb1lYeEU5dVlZc3pLNGdwcDM2?=
 =?utf-8?B?NWt1Smg2YVlEbVJleTVpTG9BL2ozRFRvUmlzamtrSlFRREJwZTgzOEpOTU50?=
 =?utf-8?B?U2VXVjlKN0dJWTlRUGpxMlBKdE1KWjNtOEEzMHhRN1Arcy9tSDZhcGJ6TWFu?=
 =?utf-8?B?WEpaaVloUkg3dHZhdEcrbzRNUzdmMm5aQjNPQmdIdEwvWUhMZENybXQ3UmFZ?=
 =?utf-8?B?VkxIbjN4SG5vcnVpdDNDVkZNdVM3S2wxOUxFTng1T0JUUm5jZmF2OXRNOUhX?=
 =?utf-8?B?MktVTWRoN1FDaEhLYWxRQ1ArNW1wVXJGdWJwd0puVHlSSkQwM2VMcEhmTEdj?=
 =?utf-8?B?aXpRS3piWTNiNUp4aE1jZnAyZlFZRU43Vk5kWkp0Z2hYTkgySU5aUG83djVF?=
 =?utf-8?B?UEg5VUlFYkR2TjlKclNiSE82U1VoRDlta1NCeXJKcG8yZUZjVzViSGVIa3pC?=
 =?utf-8?B?dWlBZEVEMlIvZnliOCtwSXYrcE1qdC9NcjJGak1EMVlYR2c3a1hCT1FjTnBi?=
 =?utf-8?B?V0tyRzVzdTh4QjBhY3EvRFF0WE5WOGdDbENBYW5iSVcxVDVJZFI3dDBHZlBT?=
 =?utf-8?B?Nm5WYVdjajBsL0wzT0FGRVM0dEZPaVRKNWY0RSs4ZVhQcTlZdGFHNEkxMDd4?=
 =?utf-8?B?RFE9PQ==?=
X-OriginatorOrg: prevas.dk
X-MS-Exchange-CrossTenant-Network-Message-Id: c18da68a-9205-4ea2-7a53-08dc26ef8d52
X-MS-Exchange-CrossTenant-AuthSource: DB9PR10MB7100.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 08:42:29.0735
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: d350cf71-778d-4780-88f5-071a4cb1ed61
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3FCxC9rDikHcH2u4qKFtLX+iEFk42ZwgojLXhPGaC1R3BV4jDqHwi5E8HrrrtlcqddCC7BGU5WXjjisQfowT6xiTcbDG/oR6P/EUtZkmnLI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR10MB6668

On 06/02/2024 00.21, Kees Cook wrote:
> 
> 
> On February 5, 2024 11:17:12 PM GMT, Eric Biggers <ebiggers@kernel.org> wrote:
>> On Mon, Feb 05, 2024 at 02:44:14PM -0800, Kees Cook wrote:
>>> On Mon, Feb 05, 2024 at 12:21:45PM -0800, Eric Biggers wrote:
>>>> On Mon, Feb 05, 2024 at 01:12:30AM -0800, Kees Cook wrote:
>>>>> Subject: Re: [PATCH v3 2/3] overflow: Introduce add_wrap(), sub_wrap(), and mul_wrap()
>>>>
>>>> Maybe these should be called wrapping_add, wrapping_sub, and wrapping_mul?
>>>> Those names are more grammatically correct, and Rust chose those names too.
>>>
>>> Sure, that works for me. What bout the inc_wrap() and dec_wrap() names?
>>> I assume wrapping_inc() and wrapping_dec() ?
>>>
>>
>> Yes, though I'm not sure those should exist at all.  Maybe a += b should just
>> become a = wrapping_add(a, b), instead of wrapping_inc(a, b)?
>> wrapping_inc(a, b) isn't as self-explanatory.  Likewise for wrapping_dec.
> 
> It was to avoid repeating type information, as it would go from:
> 
> var_a += var_b;
> 
> to:
> 
> var_a = wrapping_add(typeof(var_a), var_a, var_b);
> 
> Which repeats "var_a" 3 times. :|

Yeah, I think that's a reasonable rationale. I'm fine with the
wrapping_* naming, and then the _inc and _dec helpers should follow.

However, I now wonder if those should really also return the new value.
Yes, that corresponds to the value of the expression (a += b), but
nobody would ever write c = (a += b) or otherwise make use of that
value, and the naming doesn't immediately imply whether one should think
of ++a or a++.

Rasmus