x86/xen: Remove use of VLAs
Subsystems (2)
PATCH 1/1 x86/xen: Remove use of VLAs
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
--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
arch/x86/xen/enlighten_pv.c +2 -4
@@ -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;/*
Signature · 1 line
2.14.3
Quoted 6 lines · 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.
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
Quoted 12 lines · David Brown
Quoted 6 lines · 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.
This comment is more in regards to many of these patches, and not as much this one specifically.
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 1 line
David
Thanks,
Laura
Quoted 12 lines · David Brown
Quoted 6 lines · 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.
This comment is more in regards to many of these patches, and not as much this one specifically.
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 1 line
David
Thanks,
Laura
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 6 lines · 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.
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
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 8 lines · 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 <[email protected]>
Juergen
Quoted 8 lines · 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 <[email protected]>
Juergen
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 27 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)arch/x86/xen/enlighten_pv.c +1
-2@@ -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;/*
arch/x86/xen/enlighten_pv.c static void __init xen_load_gdt_boot(const struct desc_ptr *dtr) · line 473
Quoted 7 lines
arch/x86/xen/enlighten_pv.c +1
-2@@ -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;
Thanks,
Ingo
Quoted 27 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)arch/x86/xen/enlighten_pv.c +1
-2@@ -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;/*
arch/x86/xen/enlighten_pv.c static void __init xen_load_gdt_boot(const struct desc_ptr *dtr) · line 473
Quoted 7 lines
arch/x86/xen/enlighten_pv.c +1
-2@@ -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;
Thanks,
Ingo
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Boris Ostrovsky
reviewerQuoted 17 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)arch/x86/xen/enlighten_pv.c +0
-0
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 4 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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
Quoted 6 lines
arch/x86/xen/enlighten_pv.c +1
-1-unsigned long frames[pages]; +unsigned long frames[DIV_ROUND_UP (SZ_64K ,PAGE_SIZE )];int f;/*
arch/x86/xen/enlighten_pv.c static void __init xen_load_gdt_boot(const struct desc_ptr *dtr) · line 475
Quoted 9 lines
arch/x86/xen/enlighten_pv.c +1
-2@@ -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;/*
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Boris Ostrovsky
reviewerQuoted 17 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)arch/x86/xen/enlighten_pv.c +0
-0
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 4 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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
Quoted 15 lines
arch/x86/xen/enlighten_pv.c +2
-3-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;/*
Juergen Gross
maintainerQuoted 13 lines · Boris Ostrovsky
Quoted 12 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Quoted 13 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 9 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
GDT_SIZE - 1 :-)
Quoted 2 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-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
Quoted 28 lines · Juergen Gross
Quoted 13 lines · Boris Ostrovsky
Quoted 12 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)Quoted 13 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 9 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
Quoted 8 lines
GDT_SIZE- 1 :-)Quoted 2 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-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
Quoted 3 lines
Juergen
Boris Ostrovsky
reviewerQuoted 36 lines · Laura Abbott
Quoted 35 lines · Juergen Gross
Quoted 14 lines · Boris Ostrovsky
Quoted 13 lines · 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 <[email protected]>---
arch/x86/xen/enlighten_pv.c| 6 ++----1 file changed, 2 insertions(+), 4 deletions(-)Quoted 14 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr · line 425
Quoted 10 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
GDT_SIZE- 1 :-)Quoted 2 lines
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-1- unsigned long frames[pages]; + unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
Quoted 7 lines
Quoted 3 lines
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
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Boris Ostrovsky
reviewerQuoted 36 lines · Laura Abbott
Quoted 35 lines · Juergen Gross
Quoted 14 lines · Boris Ostrovsky
Quoted 13 lines · 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 <[email protected]>---
arch/x86/xen/enlighten_pv.c| 6 ++----1 file changed, 2 insertions(+), 4 deletions(-)Quoted 14 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr · line 425
Quoted 10 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
GDT_SIZE- 1 :-)Quoted 2 lines
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-1- unsigned long frames[pages]; + unsigned long frames[DIV_ROUND_UP(SZ_64K, PAGE_SIZE)];
Quoted 7 lines
Quoted 3 lines
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
Quoted 28 lines · Juergen Gross
Quoted 13 lines · Boris Ostrovsky
Quoted 12 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)Quoted 13 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 9 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
Quoted 8 lines
GDT_SIZE- 1 :-)Quoted 2 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-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
Quoted 3 lines
Juergen
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Juergen Gross
maintainerQuoted 13 lines · Boris Ostrovsky
Quoted 12 lines · 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 <[email protected]>--- arch/x86/xen/enlighten_pv.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Quoted 13 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 9 lines
arch/x86/xen/enlighten_pv.c +0
-1@@ -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->sizealways eitherGDT_SIZEor 0?
GDT_SIZE - 1 :-)
Quoted 2 lines
arch/x86/xen/enlighten_pv.c static void xen_load_gdt(const struct desc_ptr *dtr) · line 424
Quoted 2 lines
arch/x86/xen/enlighten_pv.c +1
-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
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel