Hi Andrew,
On Sat, Jun 01, 2024 at 06:56:15PM +0200, Erick Archer wrote:
> Hi everyone,
>
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].
>
> In the first patch, the "struct amd_uncore_ctx" can be refactored to
> use a flex array for the "events" member. This way, the allocation/
> freeing of the memory can be simplified. Then, the struct_size()
> helper can be used to do the arithmetic calculation for the memory
> to be allocated.
>
> In the second patch, as the "struct intel_uncore_box" ends in a
> flexible array, the preferred way in the kernel is to use the
> struct_size() helper to do the arithmetic instead of the calculation
> "size + count * size" in the kzalloc_node() function.
>
> In the third patch, as the "struct perf_buffer" also ends in a
> flexible array, the preferred way in the kernel is to use the
> struct_size() helper to do the arithmetic instead of the calculation
> "size + count * size" in the kzalloc_node() functions. At the same
> time, prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute.
>
> This time, I have decided to send these three patches in the same serie
> since all of them has been rejected by the maintainers. I have used
> the v4 tag since it is the latest iteration in one of the patches.
>
> The reason these patches were rejected is that Peter Zijlstra detest
> the struct_size() helper [3][4]. However, Kees Cook and I consider that
> the use of this helper improves readability. But we can all say that it
> is a matter of preference.
>
> Anyway, leaving aside personal preferences, these patches has the
> following pros:
>
> - Code robustness improvement (__counted_by coverage).
> - Code robustness improvement (use of struct_size() to do calculations
> on memory allocator functions).
> - Fewer lines of code.
> - Follow the recommendations made in "Deprecated Interfaces, Language
> Features, Attributes, and Conventions" [5] as the preferred method
> of doing things in the kernel.
> - Widely used in the kernel.
> - Widely accepted in the kernel.
>
> There are also patches in this subsystem that use the struct_size()
> helper:
>
> 6566f907bf31 ("Convert intel uncore to struct_size") by Matthew Wilcox
> dfbc411e0a5e ("perf/x86/rapl: Prefer struct_size() over open coded
> arithmetic") by me
>
> Therefore, I would like these patches to be applied this time.
This is my last attemp to get these patches applied. I have decided to
send this mail to try to unjam this situation. I have folowed all the
reviewers comments and have no response from the maintainers other than
"I detest the struct_size() helper".
Therefore, I would like to know your opinion and that of other people
about these patches. If the final consensus is that the code has no real
benefit, I will stop insisting on it ;)
Regards,
Erick
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> [1]
> Link: https://github.com/KSPP/linux/issues/160 [2]
> Link:
> https://lore.kernel.org/linux-hardening/[email protected]
> [3]
> Link:
> https://lore.kernel.org/linux-hardening/[email protected]
> [4]
> Link: https://docs.kernel.org/process/deprecated.html [5]
> ---
> Changes in v4:
>
> - Add the "Reviewed-by:" tag to the three patches.
> - Rebase against linux next (tag next-20240531).
>
> Previous versions:
>
> perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
> v1 ->
> https://lore.kernel.org/linux-hardening/as8pr02mb7237e4848b44a5226bd3551c8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
>
> perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
> v1 ->
> https://lore.kernel.org/linux-hardening/as8pr02mb7237f4d39bf6aa0ff40e91638b...@as8pr02mb7237.eurprd02.prod.outlook.com/
>
> perf/ring_buffer: Prefer struct_size over open coded arithmetic
> v3 ->
> https://lore.kernel.org/linux-hardening/as8pr02mb72379b4807f3951a1b926ba58b...@as8pr02mb7237.eurprd02.prod.outlook.com/
> v2 ->
> https://lore.kernel.org/linux-hardening/as8pr02mb7237569e4fbe0b26f62fdfdb8b...@as8pr02mb7237.eurprd02.prod.outlook.com/
> v1 ->
> https://lore.kernel.org/linux-hardening/as8pr02mb72372ab065ea8340d960ccc48b...@as8pr02mb7237.eurprd02.prod.outlook.com/
>
> Changes in v3:
> - Refactor the logic, compared to the previous version, of the second
> rb_alloc() function to gain __counted_by() coverage (Kees Cook and
> Christophe JAILLET).
>
> Changes in v2:
> - Annotate "struct perf_buffer" with __counted_by() attribute (Kees Cook).
> - Refactor the logic to gain __counted_by() coverage (Kees Cook).
> ---
> Erick Archer (3):
> perf/x86/amd/uncore: Add flex array to struct amd_uncore_ctx
> perf/x86/intel/uncore: Prefer struct_size over open coded arithmetic
> perf/ring_buffer: Prefer struct_size over open coded arithmetic
>
> arch/x86/events/amd/uncore.c | 18 +++++-------------
> arch/x86/events/intel/uncore.c | 7 +++----
> kernel/events/internal.h | 2 +-
> kernel/events/ring_buffer.c | 15 ++++-----------
> 4 files changed, 13 insertions(+), 29 deletions(-)
>
> --
> 2.25.1
>
