Re: [Intel-gfx] [PATCH] drm: i915: Adapt to -Walloc-size
Jani Nikula writes: > On Tue, 07 Nov 2023, Sam James 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 > > The short answer, > > Reviewed-by: Jani Nikula > > 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
Re: [Intel-gfx] [PATCH] drm: i915: Adapt to -Walloc-size
On Wed, 08 Nov 2023, Jani Nikula wrote: > On Tue, 07 Nov 2023, Sam James 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 > > The short answer, > > Reviewed-by: Jani Nikula 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
[Intel-gfx] [PATCH] drm: i915: Adapt to -Walloc-size
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 --- 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: [Intel-gfx] [PATCH] drm: i915: Adapt to -Walloc-size
On Tue, 07 Nov 2023, Sam James 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 The short answer, Reviewed-by: Jani Nikula 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