Re: [PATCH v4 0/3] Hardening perf subsystem

[Peter Zijlstra <peterz@infradead.org>]

On Sat, Jun 08, 2024 at 10:50:44AM +0200, Erick Archer wrote:
> 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 ;)

Seriously, I've got plenty patches to look at that actually do
something. This falls well within the 'random changes for changes sake'
and goes waaaaay down the priority list.

If you're addressing an actual issue, state so. Otherwise, go play
somewhere else.