All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	David Laight <David.Laight@ACULAB.COM>,
	'Linus Torvalds' <torvalds@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	Lucas De Marchi <lucas.demarchi@intel.com>,
	Oded Gabbay <ogabbay@kernel.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	intel-xe@lists.freedesktop.org,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-hardening@vger.kernel.org"
	<linux-hardening@vger.kernel.org>
Subject: Re: [GIT PULL] Enable -Wstringop-overflow globally
Date: Tue, 30 Jan 2024 15:52:32 +0100	[thread overview]
Message-ID: <90b41ce3-0eca-4bc9-bc04-ad68ba91d7ee@linux.intel.com> (raw)
In-Reply-To: <0c4cc1a7-f409-4597-b110-81f9e45d1ffe@embeddedor.com>

Hi,

On 1/27/24 20:53, Gustavo A. R. Silva wrote:
>
>
> On 1/27/24 09:11, David Laight wrote:
>> From: Linus Torvalds
>>> Sent: 26 January 2024 22:36
>>>
>>> On Fri, 26 Jan 2024 at 14:24, Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>> I think xe has some other weird problems too. This may be related 
>>>> (under
>>>> allocating):
>>>>
>>>> ../drivers/gpu/drm/xe/xe_vm.c: In function 'xe_vma_create':
>>>> ../drivers/gpu/drm/xe/xe_vm.c:806:21: warning: allocation of 
>>>> insufficient size '224' for type
>>> 'struct xe_vma' with size '368' [-Walloc-size]
>>>>    806 |                 vma = kzalloc(sizeof(*vma) - sizeof(struct 
>>>> xe_userptr),
>>>>        |                     ^
>>>
>>> That code is indeed odd, but there's a comment in the xe_vma definition
>>>
>>>          /**
>>>           * @userptr: user pointer state, only allocated for VMAs 
>>> that are
>>>           * user pointers
>>>           */
>>>          struct xe_userptr userptr;
>>>
>>> although I agree that it should probably simply be made a final
>>> variably-sized array instead (and then you make that array size be
>>> 0/1).
>>
>> That entire code is odd.
>> It isn't obvious that the flag values that cause the short allocate
>> are the same ones that control whether the extra data is accessed.
>>
>> Never mind the oddities with the 'flags |= ' assignments int the
>> 'remap next' path.
>>
>> Anyone know how many of these actually get allocated (and their
>> lifetimes)?
>> How much difference would it make to allocate 368 (maybe 384?)
>> bytes instead of 224 (likely 256).
>
> [CC+ xen list and maintainers]
>
> Probably the xen maintainer can help us out here.

Unfortunately the number of these can be quite large, and with a long 
lifetime which I guess was the reason that size optimization was done in 
the first place.

Ideally IMO this should've been subclassed to an xe_userptr_vma, but 
until we have a chance to clean that up, We can look at the 
variable-sized array or simply allocate the full size until we get to that.

Thanks,

Thomas


>
> -- 
> Gustavo
>
>>
>>     David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, 
>> MK1 1PT, UK
>> Registration No: 1397386 (Wales)

WARNING: multiple messages have this Message-ID (diff)
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	David Laight <David.Laight@ACULAB.COM>,
	'Linus Torvalds' <torvalds@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	Lucas De Marchi <lucas.demarchi@intel.com>,
	Oded Gabbay <ogabbay@kernel.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"linux-hardening@vger.kernel.org"
	<linux-hardening@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	intel-xe@lists.freedesktop.org,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [GIT PULL] Enable -Wstringop-overflow globally
Date: Tue, 30 Jan 2024 15:52:32 +0100	[thread overview]
Message-ID: <90b41ce3-0eca-4bc9-bc04-ad68ba91d7ee@linux.intel.com> (raw)
In-Reply-To: <0c4cc1a7-f409-4597-b110-81f9e45d1ffe@embeddedor.com>

Hi,

On 1/27/24 20:53, Gustavo A. R. Silva wrote:
>
>
> On 1/27/24 09:11, David Laight wrote:
>> From: Linus Torvalds
>>> Sent: 26 January 2024 22:36
>>>
>>> On Fri, 26 Jan 2024 at 14:24, Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>> I think xe has some other weird problems too. This may be related 
>>>> (under
>>>> allocating):
>>>>
>>>> ../drivers/gpu/drm/xe/xe_vm.c: In function 'xe_vma_create':
>>>> ../drivers/gpu/drm/xe/xe_vm.c:806:21: warning: allocation of 
>>>> insufficient size '224' for type
>>> 'struct xe_vma' with size '368' [-Walloc-size]
>>>>    806 |                 vma = kzalloc(sizeof(*vma) - sizeof(struct 
>>>> xe_userptr),
>>>>        |                     ^
>>>
>>> That code is indeed odd, but there's a comment in the xe_vma definition
>>>
>>>          /**
>>>           * @userptr: user pointer state, only allocated for VMAs 
>>> that are
>>>           * user pointers
>>>           */
>>>          struct xe_userptr userptr;
>>>
>>> although I agree that it should probably simply be made a final
>>> variably-sized array instead (and then you make that array size be
>>> 0/1).
>>
>> That entire code is odd.
>> It isn't obvious that the flag values that cause the short allocate
>> are the same ones that control whether the extra data is accessed.
>>
>> Never mind the oddities with the 'flags |= ' assignments int the
>> 'remap next' path.
>>
>> Anyone know how many of these actually get allocated (and their
>> lifetimes)?
>> How much difference would it make to allocate 368 (maybe 384?)
>> bytes instead of 224 (likely 256).
>
> [CC+ xen list and maintainers]
>
> Probably the xen maintainer can help us out here.

Unfortunately the number of these can be quite large, and with a long 
lifetime which I guess was the reason that size optimization was done in 
the first place.

Ideally IMO this should've been subclassed to an xe_userptr_vma, but 
until we have a chance to clean that up, We can look at the 
variable-sized array or simply allocate the full size until we get to that.

Thanks,

Thomas


>
> -- 
> Gustavo
>
>>
>>     David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, 
>> MK1 1PT, UK
>> Registration No: 1397386 (Wales)

  reply	other threads:[~2024-01-30 14:52 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-22 15:29 [GIT PULL] Enable -Wstringop-overflow globally Gustavo A. R. Silva
2024-01-22 18:02 ` pr-tracker-bot
2024-01-26 21:22 ` Linus Torvalds
2024-01-26 21:30   ` Gustavo A. R. Silva
2024-01-26 22:24     ` Kees Cook
2024-01-26 22:36       ` Linus Torvalds
2024-01-27 15:11         ` David Laight
2024-01-27 19:53           ` Gustavo A. R. Silva
2024-01-27 19:53             ` Gustavo A. R. Silva
2024-01-30 14:52             ` Thomas Hellström [this message]
2024-01-30 14:52               ` Thomas Hellström
2024-02-02  7:52   ` Arnd Bergmann
2024-02-02 18:29     ` Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=90b41ce3-0eca-4bc9-bc04-ad68ba91d7ee@linux.intel.com \
    --to=thomas.hellstrom@linux.intel.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gustavo@embeddedor.com \
    --cc=gustavoars@kernel.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lucas.demarchi@intel.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=ogabbay@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.