[PATCH] drm: i915: Adapt to -Walloc-size

[PATCH] drm: i915: Adapt to -Walloc-size

[Sam James]

GCC 14 introduces a new -Walloc-size included in -Wextra which errors out
like:
```
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: In function ‘eb_copy_relocations’:
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1681:24: error: allocation of insufficient size ‘1’ for type ‘struct drm_i915_gem_relocation_entry’ with size ‘32’ [-Werror=alloc-size]
 1681 |                 relocs = kvmalloc_array(size, 1, GFP_KERNEL);
      |                        ^

```

So, just swap the number of members and size arguments to match the prototype, as
we're initialising 1 element of size `size`. GCC then sees we're not
doing anything wrong.

Signed-off-by: Sam James <sam@gentoo.org>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 683fd8d3151c..45b9d9e34b8b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1678,7 +1678,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
 		urelocs = u64_to_user_ptr(eb->exec[i].relocs_ptr);
 		size = nreloc * sizeof(*relocs);
 
-		relocs = kvmalloc_array(size, 1, GFP_KERNEL);
+		relocs = kvmalloc_array(1, size, GFP_KERNEL);
 		if (!relocs) {
 			err = -ENOMEM;
 			goto err;
-- 
2.42.1

Re: [PATCH] drm: i915: Adapt to -Walloc-size

[Jani Nikula]

On Tue, 07 Nov 2023, Sam James <sam@gentoo.org> wrote:
> GCC 14 introduces a new -Walloc-size included in -Wextra which errors out
> like:
> ```
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: In function ‘eb_copy_relocations’:
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1681:24: error: allocation of insufficient size ‘1’ for type ‘struct drm_i915_gem_relocation_entry’ with size ‘32’ [-Werror=alloc-size]
>  1681 |                 relocs = kvmalloc_array(size, 1, GFP_KERNEL);
>       |                        ^
>
> ```
>
> So, just swap the number of members and size arguments to match the prototype, as
> we're initialising 1 element of size `size`. GCC then sees we're not
> doing anything wrong.
>
> Signed-off-by: Sam James <sam@gentoo.org>

The short answer,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

but please read on.

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 683fd8d3151c..45b9d9e34b8b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1678,7 +1678,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
>  		urelocs = u64_to_user_ptr(eb->exec[i].relocs_ptr);
>  		size = nreloc * sizeof(*relocs);
>  
> -		relocs = kvmalloc_array(size, 1, GFP_KERNEL);
> +		relocs = kvmalloc_array(1, size, GFP_KERNEL);

Based on the patch context, we should really be calling:

	kvmalloc_array(nreloc, sizeof(*relocs), GFP_KERNEL);

and we'd get mul overflow checks too.

However, the code below also needs size, unless it's refactored to
operate on multiples of sizeof(*relocs) and it all gets a bit annoying.

Maybe there's a better way, but for the short term the patch at hand is
no worse than what we currently have, and it'll silence the warning, so
let's go with this.


>  		if (!relocs) {
>  			err = -ENOMEM;
>  			goto err;

-- 
Jani Nikula, Intel

Re: [PATCH] drm: i915: Adapt to -Walloc-size

[Jani Nikula]

On Wed, 08 Nov 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 07 Nov 2023, Sam James <sam@gentoo.org> wrote:
>> GCC 14 introduces a new -Walloc-size included in -Wextra which errors out
>> like:
>> ```
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: In function ‘eb_copy_relocations’:
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1681:24: error: allocation of insufficient size ‘1’ for type ‘struct drm_i915_gem_relocation_entry’ with size ‘32’ [-Werror=alloc-size]
>>  1681 |                 relocs = kvmalloc_array(size, 1, GFP_KERNEL);
>>       |                        ^
>>
>> ```
>>
>> So, just swap the number of members and size arguments to match the prototype, as
>> we're initialising 1 element of size `size`. GCC then sees we're not
>> doing anything wrong.
>>
>> Signed-off-by: Sam James <sam@gentoo.org>
>
> The short answer,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

And pushed to drm-intel-gt-next, thanks for the patch.

BR,
Jani.

>
> but please read on.
>
>> ---
>>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 683fd8d3151c..45b9d9e34b8b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -1678,7 +1678,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
>>  		urelocs = u64_to_user_ptr(eb->exec[i].relocs_ptr);
>>  		size = nreloc * sizeof(*relocs);
>>  
>> -		relocs = kvmalloc_array(size, 1, GFP_KERNEL);
>> +		relocs = kvmalloc_array(1, size, GFP_KERNEL);
>
> Based on the patch context, we should really be calling:
>
> 	kvmalloc_array(nreloc, sizeof(*relocs), GFP_KERNEL);
>
> and we'd get mul overflow checks too.
>
> However, the code below also needs size, unless it's refactored to
> operate on multiples of sizeof(*relocs) and it all gets a bit annoying.
>
> Maybe there's a better way, but for the short term the patch at hand is
> no worse than what we currently have, and it'll silence the warning, so
> let's go with this.
>
>
>>  		if (!relocs) {
>>  			err = -ENOMEM;
>>  			goto err;

-- 
Jani Nikula, Intel

Re: [PATCH] drm: i915: Adapt to -Walloc-size

[Sam James]

Jani Nikula <jani.nikula@linux.intel.com> writes:

> On Tue, 07 Nov 2023, Sam James <sam@gentoo.org> wrote:
>> GCC 14 introduces a new -Walloc-size included in -Wextra which errors out
>> like:
>> ```
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: In function ‘eb_copy_relocations’:
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1681:24: error: allocation of insufficient size ‘1’ for type ‘struct drm_i915_gem_relocation_entry’ with size ‘32’ [-Werror=alloc-size]
>>  1681 |                 relocs = kvmalloc_array(size, 1, GFP_KERNEL);
>>       |                        ^
>>
>> ```
>>
>> So, just swap the number of members and size arguments to match the prototype, as
>> we're initialising 1 element of size `size`. GCC then sees we're not
>> doing anything wrong.
>>
>> Signed-off-by: Sam James <sam@gentoo.org>
>
> The short answer,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> but please read on.
>
>> ---
>>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 683fd8d3151c..45b9d9e34b8b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -1678,7 +1678,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
>>  		urelocs = u64_to_user_ptr(eb->exec[i].relocs_ptr);
>>  		size = nreloc * sizeof(*relocs);
>>  
>> -		relocs = kvmalloc_array(size, 1, GFP_KERNEL);
>> +		relocs = kvmalloc_array(1, size, GFP_KERNEL);
>
> Based on the patch context, we should really be calling:
>
> 	kvmalloc_array(nreloc, sizeof(*relocs), GFP_KERNEL);
>
> and we'd get mul overflow checks too.
>
> However, the code below also needs size, unless it's refactored to
> operate on multiples of sizeof(*relocs) and it all gets a bit annoying.
>
> Maybe there's a better way, but for the short term the patch at hand is
> no worse than what we currently have, and it'll silence the warning, so
> let's go with this.

Thanks. I have been trying to port to kvmalloc_array where I can if it's
obvious/trivial, but I admit I didn't want to take it on when it'd
require any surrounding refactoring unless someone insisted.

>
>
>>  		if (!relocs) {
>>  			err = -ENOMEM;
>>  			goto err;

best,
sam