Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 16 Oct 2021, Len Baker <len.baker@gmx.com> wrote:
> Hi Daniel and Jani,
>
> On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote:
>> On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote:
>> > On Mon, 11 Oct 2021, Len Baker <len.baker@gmx.com> wrote:
>> > > Hi,
>> > >
>> > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote:
>> > >> As noted in the "Deprecated Interfaces, Language Features, Attributes,
>> > >> and Conventions" documentation [1], size calculations (especially
>> > >> multiplication) should not be performed in memory allocator (or similar)
>> > >> function arguments due to the risk of them overflowing. This could lead
>> > >> to values wrapping around and a smaller allocation being made than the
>> > >> caller was expecting. Using those allocations could lead to linear
>> > >> overflows of heap memory and other misbehaviors.
>> > >>
>> > >> In this case these are not actually dynamic sizes: all the operands
>> > >> involved in the calculation are constant values. However it is better to
>> > >> refactor them anyway, just to keep the open-coded math idiom out of
>> > >> code.
>> > >>
>> > >> So, add at the end of the struct i915_syncmap a union with two flexible
>> > >> array members (these arrays share the same memory layout). This is
>> > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the
>> > >> struct_size() helper to do the arithmetic instead of the argument
>> > >> "size + count * size" in the kmalloc and kzalloc() functions.
>> > >>
>> > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child
>> > >> making them more readable.
>> > >>
>> > >> This code was detected with the help of Coccinelle and audited and fixed
>> > >> manually.
>> > >>
>> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
>> > >>
>> > >> Signed-off-by: Len Baker <len.baker@gmx.com>
>> > >> ---
>> > >>  drivers/gpu/drm/i915/i915_syncmap.c | 12 ++++++++----
>> > >>  1 file changed, 8 insertions(+), 4 deletions(-)
>> > >
>> > > I received a mail telling that this patch doesn't build:
>> > >
>> > > == Series Details ==
>> > >
>> > > Series: drm/i915: Prefer struct_size over open coded arithmetic
>> > > URL   : https://patchwork.freedesktop.org/series/95408/
>> > > State : failure
>> > >
>> > > But it builds without error against linux-next (tag next-20211001). Against
>> > > which tree and branch do I need to build?
>> >
>> > drm-tip [1]. It's a sort of linux-next for graphics. I think there are
>> > still some branches that don't feed to linux-next.
>>
>> Yeah we need to get gt-next in linux-next asap. Joonas promised to send
>> out his patch to make that happen in dim.
>> -Daniel
>
> Is there a possibility that you give an "Acked-by" tag? And then this patch
> goes to the mainline through the Kees' tree or Gustavo's tree?

If this does not apply to drm-intel-gt-next (or drm-tip), applying it to
some other branch will just cause unnecessary conflicts later on. It's
unnecessary extra work. It's not an urgent fix or anything, there is no
reason to do that. So that's a NAK.

> Or is it better to wait for drm-tip to update?

drm-tip is up to date, it's just that one of the branches that feed to
it is (was?) not feeding to linux-next.

If you're contributing to drm, please consider basing your patches on
top of drm-tip.


BR,
Jani.


>
> Regards,
> Len
>
>>
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> > [1] https://cgit.freedesktop.org/drm/drm-tip
>> >
>> >
>> > >
>> > > Regards,
>> > > Len
>> >
>> > --
>> > Jani Nikula, Intel Open Source Graphics Center
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Graphics Center