[PATCH] x86/xen: Remove use of VLAs

[PATCH] x86/xen: Remove use of VLAs

[Laura Abbott]

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The few VLAs in use have an upper bound based on a size
of 64K. This doesn't produce an excessively large stack so just switch
the upper bound.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 arch/x86/xen/enlighten_pv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c36d23aa6c35..d96a5a535cbb 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr *dtr)
 {
 	unsigned long va = dtr->address;
 	unsigned int size = dtr->size + 1;
-	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
-	unsigned long frames[pages];
+	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
 	int f;
 
 	/*
@@ -470,8 +469,7 @@ static void __init xen_load_gdt_boot(const struct desc_ptr *dtr)
 {
 	unsigned long va = dtr->address;
 	unsigned int size = dtr->size + 1;
-	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
-	unsigned long frames[pages];
+	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
 	int f;
 
 	/*
-- 
2.14.3

Re: [PATCH] x86/xen: Remove use of VLAs

[David Brown]

On Fri, Apr 13, 2018 at 03:11:46PM -0700, Laura Abbott wrote:

>There's an ongoing effort to remove VLAs[1] from the kernel to eventually
>turn on -Wvla. The few VLAs in use have an upper bound based on a size
>of 64K. This doesn't produce an excessively large stack so just switch
>the upper bound.
>
>[1] https://lkml.org/lkml/2018/3/7/621

This comment is more in regards to many of these patches, and not as
much this one specifically.

How confident are we in the upper bounds we're setting, and how
obvious is it in the resulting code so that something does later
change to overflow these bounds.

The danger here is that we're converting something a little easier to
detect (a stack overflow), with something harder to detect
(overflowing an array on the stack).

I guess the question is twofold: how did you determine that 64K was
the largest 'size' value, and how should reviewers verify this as
well.  Perhaps this should at least be in the commit text so someone
tracking down something with this code can find it later.

David

Re: [PATCH] x86/xen: Remove use of VLAs

[Laura Abbott]

On 04/13/2018 07:55 PM, David Brown wrote:
> On Fri, Apr 13, 2018 at 03:11:46PM -0700, Laura Abbott wrote:
> 
>> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
>> turn on -Wvla. The few VLAs in use have an upper bound based on a size
>> of 64K. This doesn't produce an excessively large stack so just switch
>> the upper bound.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
> 
> This comment is more in regards to many of these patches, and not as
> much this one specifically.
> 
> How confident are we in the upper bounds we're setting, and how
> obvious is it in the resulting code so that something does later
> change to overflow these bounds.
> 
> The danger here is that we're converting something a little easier to
> detect (a stack overflow), with something harder to detect
> (overflowing an array on the stack).
> 

Several people have remarked on that and the solution has been to
put in some kind of WARN and/or error check to make it obvious something
needs to be adjusted.

> I guess the question is twofold: how did you determine that 64K was
> the largest 'size' value, and how should reviewers verify this as
> well.  Perhaps this should at least be in the commit text so someone
> tracking down something with this code can find it later.
> 

It's not in the patch context but there's a large comment below:

         /*
          * A GDT can be up to 64k in size, which corresponds to 8192
          * 8-byte entries, or 16 4k pages..
          */

         BUG_ON(size > 65536);


Given the frames was calculated based off the size, that seemed
sufficient.

> David

Thanks,
Laura

Re: [PATCH] x86/xen: Remove use of VLAs

[Juergen Gross]

On 14/04/18 00:11, Laura Abbott wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. The few VLAs in use have an upper bound based on a size
> of 64K. This doesn't produce an excessively large stack so just switch
> the upper bound.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH] x86/xen: Remove use of VLAs

[Ingo Molnar]

* Laura Abbott <labbott@redhat.com> wrote:

> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. The few VLAs in use have an upper bound based on a size
> of 64K. This doesn't produce an excessively large stack so just switch
> the upper bound.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  arch/x86/xen/enlighten_pv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c36d23aa6c35..d96a5a535cbb 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr *dtr)
>  {
>  	unsigned long va = dtr->address;
>  	unsigned int size = dtr->size + 1;
> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
> -	unsigned long frames[pages];
> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
>  	int f;
>  
>  	/*
> @@ -470,8 +469,7 @@ static void __init xen_load_gdt_boot(const struct desc_ptr *dtr)
>  {
>  	unsigned long va = dtr->address;
>  	unsigned int size = dtr->size + 1;
> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
> -	unsigned long frames[pages];
> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
>  	int f;

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH] x86/xen: Remove use of VLAs

[Boris Ostrovsky]

On 04/13/2018 06:11 PM, Laura Abbott wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. The few VLAs in use have an upper bound based on a size
> of 64K. This doesn't produce an excessively large stack so just switch
> the upper bound.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  arch/x86/xen/enlighten_pv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c36d23aa6c35..d96a5a535cbb 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr *dtr)
>  {
>  	unsigned long va = dtr->address;
>  	unsigned int size = dtr->size + 1;
> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);



Isn't dtr->size always either GDT_SIZE or 0?

-boris




> -	unsigned long frames[pages];
> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
>  	int f;
>  
>  	/*
> @@ -470,8 +469,7 @@ static void __init xen_load_gdt_boot(const struct desc_ptr *dtr)
>  {
>  	unsigned long va = dtr->address;
>  	unsigned int size = dtr->size + 1;
> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
> -	unsigned long frames[pages];
> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
>  	int f;
>  
>  	/*

Re: [PATCH] x86/xen: Remove use of VLAs

[Juergen Gross]

On 16/04/18 15:27, Boris Ostrovsky wrote:
> On 04/13/2018 06:11 PM, Laura Abbott wrote:
>> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
>> turn on -Wvla. The few VLAs in use have an upper bound based on a size
>> of 64K. This doesn't produce an excessively large stack so just switch
>> the upper bound.
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>  arch/x86/xen/enlighten_pv.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index c36d23aa6c35..d96a5a535cbb 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr *dtr)
>>  {
>>  	unsigned long va = dtr->address;
>>  	unsigned int size = dtr->size + 1;
>> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
> 
> 
> 
> Isn't dtr->size always either GDT_SIZE or 0?

GDT_SIZE - 1 :-)

>> -	unsigned long frames[pages];
>> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];

So we could just go with one frame and modify the BUG_ON() further below
accordingly.


Juergen

Re: [PATCH] x86/xen: Remove use of VLAs

[Laura Abbott]

On 04/17/2018 12:16 AM, Juergen Gross wrote:
> On 16/04/18 15:27, Boris Ostrovsky wrote:
>> On 04/13/2018 06:11 PM, Laura Abbott wrote:
>>> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
>>> turn on -Wvla. The few VLAs in use have an upper bound based on a size
>>> of 64K. This doesn't produce an excessively large stack so just switch
>>> the upper bound.
>>>
>>> [1] https://lkml.org/lkml/2018/3/7/621
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>>   arch/x86/xen/enlighten_pv.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>>> index c36d23aa6c35..d96a5a535cbb 100644
>>> --- a/arch/x86/xen/enlighten_pv.c
>>> +++ b/arch/x86/xen/enlighten_pv.c
>>> @@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr *dtr)
>>>   {
>>>   	unsigned long va = dtr->address;
>>>   	unsigned int size = dtr->size + 1;
>>> -	unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
>>
>>
>>
>> Isn't dtr->size always either GDT_SIZE or 0?
> 
> GDT_SIZE - 1 :-)
> 
>>> -	unsigned long frames[pages];
>>> +	unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
> 
> So we could just go with one frame and modify the BUG_ON() further below
> accordingly.
> 

Do you want to just remove the loop as well since we're never going
to do more than one frame? We end up with net code deletion.

Thanks,
Laura

> 
> Juergen
> 

Re: [PATCH] x86/xen: Remove use of VLAs

[Boris Ostrovsky]

On 04/17/2018 07:33 PM, Laura Abbott wrote:
> On 04/17/2018 12:16 AM, Juergen Gross wrote:
>> On 16/04/18 15:27, Boris Ostrovsky wrote:
>>> On 04/13/2018 06:11 PM, Laura Abbott wrote:
>>>> There's an ongoing effort to remove VLAs[1] from the kernel to
>>>> eventually
>>>> turn on -Wvla. The few VLAs in use have an upper bound based on a size
>>>> of 64K. This doesn't produce an excessively large stack so just switch
>>>> the upper bound.
>>>>
>>>> [1] https://lkml.org/lkml/2018/3/7/621
>>>>
>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>> ---
>>>>   arch/x86/xen/enlighten_pv.c | 6 ++----
>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>>>> index c36d23aa6c35..d96a5a535cbb 100644
>>>> --- a/arch/x86/xen/enlighten_pv.c
>>>> +++ b/arch/x86/xen/enlighten_pv.c
>>>> @@ -421,8 +421,7 @@ static void xen_load_gdt(const struct desc_ptr
>>>> *dtr)
>>>>   {
>>>>       unsigned long va = dtr->address;
>>>>       unsigned int size = dtr->size + 1;
>>>> -    unsigned pages = DIV_ROUND_UP(size, PAGE_SIZE);
>>>
>>>
>>>
>>> Isn't dtr->size always either GDT_SIZE or 0?
>>
>> GDT_SIZE - 1 :-)
>>
>>>> -    unsigned long frames[pages];
>>>> +    unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
>>
>> So we could just go with one frame and modify the BUG_ON() further below
>> accordingly.
>>
>
> Do you want to just remove the loop as well since we're never going
> to do more than one frame? We end up with net code deletion.
>


Yes, the loop, as well as the comment about max size being 64K can all
be removed.

-boris