[PATCH 0/8] Finish effort to decrease maximum stack frame to 2048
Few outstanding patches of code compiled only on FreeBSD Pipeline: https://gitlab.com/pipo.sk/libvirt/-/pipelines/986470469 Peter Krempa (8): bhyve: Don't stack-allocate huge error buffers virHostValidateBhyve: Declare one variable per line virHostValidateBhyve: Heap allocate massive 'struct kld_file_stat' nss: aiforaf: Format one argument/variable per line nss: aiforaf: Remove unused 'ret' variable nss: aiforaf: Drop unused buffer 'port' nss: aiforaf: Decrease stack size by scoping off large buffers. build: Decrease maximum stack frame size to 2048 meson.build | 2 +- src/bhyve/bhyve_process.c | 4 +- tools/nss/libvirt_nss.c | 105 ++++++++++++++++++------------- tools/virt-host-validate-bhyve.c | 20 +++--- 4 files changed, 74 insertions(+), 57 deletions(-) -- 2.41.0
_POSIX2_LINE_MAX is 2048. Allocate the buffers on the heap instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/bhyve/bhyve_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 80d5a8804f..c8e1a10afe 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -380,7 +380,7 @@ virBhyveGetDomainTotalCpuStats(virDomainObj *vm, { struct kinfo_proc *kp; kvm_t *kd; - char errbuf[_POSIX2_LINE_MAX]; + g_autofree char *errbuf = g_new0(char, _POSIX2_LINE_MAX); int nprocs; int ret = -1; @@ -481,7 +481,7 @@ virBhyveProcessReconnectAll(struct _bhyveConn *driver) { kvm_t *kd; struct bhyveProcessReconnectData data; - char errbuf[_POSIX2_LINE_MAX]; + g_autofree char *errbuf = g_new0(char, _POSIX2_LINE_MAX); if ((kd = kvm_openfiles(NULL, NULL, NULL, O_RDONLY, errbuf)) == NULL) { virReportError(VIR_ERR_SYSTEM_ERROR, -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
_POSIX2_LINE_MAX is 2048. Allocate the buffers on the heap instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/bhyve/bhyve_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-host-validate-bhyve.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/virt-host-validate-bhyve.c b/tools/virt-host-validate-bhyve.c index a39225016d..9457fac9a5 100644 --- a/tools/virt-host-validate-bhyve.c +++ b/tools/virt-host-validate-bhyve.c @@ -50,8 +50,10 @@ int virHostValidateBhyve(void) int ret = 0; int fileid = 0; struct kld_file_stat stat; - bool vmm_loaded = false, if_tap_loaded = false; - bool if_bridge_loaded = false, nmdm_loaded = false; + bool vmm_loaded = false; + bool if_tap_loaded = false; + bool if_bridge_loaded = false; + bool nmdm_loaded = false; for (fileid = kldnext(0); fileid > 0; fileid = kldnext(fileid)) { stat.version = sizeof(struct kld_file_stat); -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-host-validate-bhyve.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-host-validate-bhyve.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virt-host-validate-bhyve.c b/tools/virt-host-validate-bhyve.c index 9457fac9a5..db1cdd8e2c 100644 --- a/tools/virt-host-validate-bhyve.c +++ b/tools/virt-host-validate-bhyve.c @@ -49,24 +49,24 @@ int virHostValidateBhyve(void) { int ret = 0; int fileid = 0; - struct kld_file_stat stat; + g_autofree struct kld_file_stat *stat = g_new0(struct kld_file_stat, 1); bool vmm_loaded = false; bool if_tap_loaded = false; bool if_bridge_loaded = false; bool nmdm_loaded = false; for (fileid = kldnext(0); fileid > 0; fileid = kldnext(fileid)) { - stat.version = sizeof(struct kld_file_stat); - if (kldstat(fileid, &stat) < 0) + stat->version = sizeof(struct kld_file_stat); + if (kldstat(fileid, stat) < 0) continue; - if (STREQ(stat.name, "vmm.ko")) + if (STREQ(stat->name, "vmm.ko")) vmm_loaded = true; - else if (STREQ(stat.name, "if_tap.ko")) + else if (STREQ(stat->name, "if_tap.ko")) if_tap_loaded = true; - else if (STREQ(stat.name, "if_bridge.ko")) + else if (STREQ(stat->name, "if_bridge.ko")) if_bridge_loaded = true; - else if (STREQ(stat.name, "nmdm.ko")) + else if (STREQ(stat->name, "nmdm.ko")) nmdm_loaded = true; } -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-host-validate-bhyve.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
Break up the argument and variable declarations to the preferred style. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index b028f28608..cd2b8feb5a 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -464,13 +464,19 @@ ns_mtab methods[] = { }; static void -aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip) +aiforaf(const char *name, + int af, + struct addrinfo *pai, + struct addrinfo **aip) { int ret; struct hostent resolved; char buf[1024] = { 0 }; - int err, herr; - struct addrinfo hints, *res0, *res; + int err; + int herr; + struct addrinfo hints; + struct addrinfo *res0; + struct addrinfo *res; char **addrList; if ((ret = NSS_NAME(gethostbyname2)(name, af, &resolved, -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
Break up the argument and variable declarations to the preferred style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
The variable is set but never actually used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index cd2b8feb5a..faa44e78df 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -469,7 +469,6 @@ aiforaf(const char *name, struct addrinfo *pai, struct addrinfo **aip) { - int ret; struct hostent resolved; char buf[1024] = { 0 }; int err; @@ -479,9 +478,9 @@ aiforaf(const char *name, struct addrinfo *res; char **addrList; - if ((ret = NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr)) != NS_SUCCESS) + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr) != NS_SUCCESS) return; addrList = resolved.h_addr_list; -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
The variable is set but never actually used.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
The 'port' buffer is passed to 'getnameinfo' which is supposed to fill it but it's not actually later used. Drop the buffer as 'getnameinfo' allows NULL arguments if they are not needed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index faa44e78df..37720bf4ae 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -493,7 +493,6 @@ aiforaf(const char *name, socklen_t salen; void *address = *addrList; char host[NI_MAXHOST]; - char port[NI_MAXSERV]; if (resolved.h_addrtype == AF_INET) { sa.sin.sin_family = AF_INET; @@ -511,7 +510,7 @@ aiforaf(const char *name, if ((err = getnameinfo(&sa.sa, salen, host, sizeof(host), - port, sizeof(port), + NULL, 0, NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { ERROR("Cannot convert socket address to string: %s", gai_strerror(err)); -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
The 'port' buffer is passed to 'getnameinfo' which is supposed to fill it but it's not actually later used. Drop the buffer as 'getnameinfo' allows NULL arguments if they are not needed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks. In this instance we do not want to use dynamic allocation as this is the NSS module. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 97 +++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 43 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 37720bf4ae..dff3c034bf 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -470,61 +470,72 @@ aiforaf(const char *name, struct addrinfo **aip) { struct hostent resolved; - char buf[1024] = { 0 }; int err; - int herr; - struct addrinfo hints; - struct addrinfo *res0; - struct addrinfo *res; char **addrList; - if (NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr) != NS_SUCCESS) - return; + /* Note: The do-while blocks in this function are used to scope off large + * stack allocated buffers, which are not needed at the same time */ + do { + char buf[1024] = { 0 }; + int herr; + + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr) != NS_SUCCESS) + return; + } while (false); addrList = resolved.h_addr_list; while (*addrList) { - union { - struct sockaddr sa; - struct sockaddr_in sin; - struct sockaddr_in6 sin6; - } sa = { 0 }; - socklen_t salen; void *address = *addrList; char host[NI_MAXHOST]; + struct addrinfo *res0; + struct addrinfo *res; + + do { + union { + struct sockaddr sa; + struct sockaddr_in sin; + struct sockaddr_in6 sin6; + } sa = { 0 }; + socklen_t salen; + + if (resolved.h_addrtype == AF_INET) { + sa.sin.sin_family = AF_INET; + memcpy(&sa.sin.sin_addr.s_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET)); + salen = sizeof(sa.sin); + } else { + sa.sin6.sin6_family = AF_INET6; + memcpy(&sa.sin6.sin6_addr.s6_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET6)); + salen = sizeof(sa.sin6); + } - if (resolved.h_addrtype == AF_INET) { - sa.sin.sin_family = AF_INET; - memcpy(&sa.sin.sin_addr.s_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET)); - salen = sizeof(sa.sin); - } else { - sa.sin6.sin6_family = AF_INET6; - memcpy(&sa.sin6.sin6_addr.s6_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET6)); - salen = sizeof(sa.sin6); - } + if ((err = getnameinfo(&sa.sa, salen, + host, sizeof(host), + NULL, 0, + NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { + ERROR("Cannot convert socket address to string: %s", + gai_strerror(err)); + continue; + } + } while (false); - if ((err = getnameinfo(&sa.sa, salen, - host, sizeof(host), - NULL, 0, - NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { - ERROR("Cannot convert socket address to string: %s", - gai_strerror(err)); - continue; - } + do { + struct addrinfo hints; - hints = *pai; - hints.ai_flags = AI_NUMERICHOST; - hints.ai_family = af; + hints = *pai; + hints.ai_flags = AI_NUMERICHOST; + hints.ai_family = af; - if (getaddrinfo(host, NULL, &hints, &res0)) { - addrList++; - continue; - } + if (getaddrinfo(host, NULL, &hints, &res0)) { + addrList++; + continue; + } + } while (false); for (res = res0; res; res = res->ai_next) res->ai_flags = pai->ai_flags; -- 2.41.0
On Wed, Aug 30, 2023 at 13:59:21 +0200, Peter Krempa wrote:
The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks.
In this instance we do not want to use dynamic allocation as this is the NSS module.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 97 +++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 43 deletions(-)
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 37720bf4ae..dff3c034bf 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c
[...]
- hints = *pai; - hints.ai_flags = AI_NUMERICHOST; - hints.ai_family = af; + hints = *pai; + hints.ai_flags = AI_NUMERICHOST; + hints.ai_family = af;
- if (getaddrinfo(host, NULL, &hints, &res0)) { - addrList++; - continue;
Ehh, self-NACK ...
- } + if (getaddrinfo(host, NULL, &hints, &res0)) { + addrList++; + continue; + } + } while (false);
for (res = res0; res; res = res->ai_next) res->ai_flags = pai->ai_flags; -- 2.41.0
On Wed, Aug 30, 2023 at 2:21 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Wed, Aug 30, 2023 at 13:59:21 +0200, Peter Krempa wrote:
The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks.
In this instance we do not want to use dynamic allocation as this is the NSS module.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/nss/libvirt_nss.c | 97 +++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 43 deletions(-)
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 37720bf4ae..dff3c034bf 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c
[...]
- hints = *pai; - hints.ai_flags = AI_NUMERICHOST; - hints.ai_family = af; + hints = *pai; + hints.ai_flags = AI_NUMERICHOST; + hints.ai_family = af;
- if (getaddrinfo(host, NULL, &hints, &res0)) { - addrList++; - continue;
Ehh, self-NACK ...
Apart from moving these two continue into loops...
- } + if (getaddrinfo(host, NULL, &hints, &res0)) { + addrList++; + continue; + } + } while (false);
for (res = res0; res; res = res->ai_next) res->ai_flags = pai->ai_flags; -- 2.41.0
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks. In this instance we do not want to use dynamic allocation as this is the NSS module. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- v2: Moved two error checks containing a 'continue' out of the 'do-while' loop blocks New pipeline: https://gitlab.com/pipo.sk/libvirt/-/pipelines/986529301 tools/nss/libvirt_nss.c | 87 ++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 36 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 37720bf4ae..ff6ea1d373 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -470,58 +470,73 @@ aiforaf(const char *name, struct addrinfo **aip) { struct hostent resolved; - char buf[1024] = { 0 }; int err; - int herr; - struct addrinfo hints; - struct addrinfo *res0; - struct addrinfo *res; char **addrList; - if (NSS_NAME(gethostbyname2)(name, af, &resolved, - buf, sizeof(buf), - &err, &herr) != NS_SUCCESS) - return; + /* Note: The do-while blocks in this function are used to scope off large + * stack allocated buffers, which are not needed at the same time */ + do { + char buf[1024] = { 0 }; + int herr; + + if (NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr) != NS_SUCCESS) + return; + } while (false); addrList = resolved.h_addr_list; while (*addrList) { - union { - struct sockaddr sa; - struct sockaddr_in sin; - struct sockaddr_in6 sin6; - } sa = { 0 }; - socklen_t salen; void *address = *addrList; char host[NI_MAXHOST]; + struct addrinfo *res0; + struct addrinfo *res; + + do { + union { + struct sockaddr sa; + struct sockaddr_in sin; + struct sockaddr_in6 sin6; + } sa = { 0 }; + socklen_t salen; + + if (resolved.h_addrtype == AF_INET) { + sa.sin.sin_family = AF_INET; + memcpy(&sa.sin.sin_addr.s_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET)); + salen = sizeof(sa.sin); + } else { + sa.sin6.sin6_family = AF_INET6; + memcpy(&sa.sin6.sin6_addr.s6_addr, + address, + FAMILY_ADDRESS_SIZE(AF_INET6)); + salen = sizeof(sa.sin6); + } - if (resolved.h_addrtype == AF_INET) { - sa.sin.sin_family = AF_INET; - memcpy(&sa.sin.sin_addr.s_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET)); - salen = sizeof(sa.sin); - } else { - sa.sin6.sin6_family = AF_INET6; - memcpy(&sa.sin6.sin6_addr.s6_addr, - address, - FAMILY_ADDRESS_SIZE(AF_INET6)); - salen = sizeof(sa.sin6); - } + err = getnameinfo(&sa.sa, salen, + host, sizeof(host), + NULL, 0, + NI_NUMERICHOST | NI_NUMERICSERV); + } while (false); - if ((err = getnameinfo(&sa.sa, salen, - host, sizeof(host), - NULL, 0, - NI_NUMERICHOST | NI_NUMERICSERV)) != 0) { + if (err != 0) { ERROR("Cannot convert socket address to string: %s", gai_strerror(err)); continue; } - hints = *pai; - hints.ai_flags = AI_NUMERICHOST; - hints.ai_family = af; + do { + struct addrinfo hints; + + hints = *pai; + hints.ai_flags = AI_NUMERICHOST; + hints.ai_family = af; + + err = getaddrinfo(host, NULL, &hints, &res0); + } while (false); - if (getaddrinfo(host, NULL, &hints, &res0)) { + if (err != 0) { addrList++; continue; } -- 2.41.0
On Wed, Aug 30, 2023 at 3:17 PM Peter Krempa <pkrempa@redhat.com> wrote:
The 'buf', 'sa' and 'hints' stack allocated helper variables are never used together. Decrease the stack memory usage by scoping them off into do-while blocks.
In this instance we do not want to use dynamic allocation as this is the NSS module.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
v2: Moved two error checks containing a 'continue' out of the 'do-while' loop blocks
New pipeline: https://gitlab.com/pipo.sk/libvirt/-/pipelines/986529301
tools/nss/libvirt_nss.c | 87 ++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 36 deletions(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
After recent cleanups we can now restrict the maximum stack frame size to 2k. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( ) # sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768 # array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with -- 2.41.0
On Wed, Aug 30, 2023 at 1:59 PM Peter Krempa <pkrempa@redhat.com> wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Kristina Hanicova <khanicov@redhat.com> Kristina
On 8/30/23 13:59, Peter Krempa wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( )
# sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
# array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really understand why this is needed. I mean, we certainly do not want large frames, but IIUC only frames larger than a page are problem (i.e. 4KiB). Can you please point me to some docs where I can learn more? Michal
On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote:
On 8/30/23 13:59, Peter Krempa wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( )
# sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
# array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really understand why this is needed. I mean, we certainly do not want large frames, but IIUC only frames larger than a page are problem (i.e. 4KiB). Can you please point me to some docs where I can learn more?
The main idea of this is to ensure that we don't have massive stack frames which could cause a stack overflow. But to be honest, it doesn't matter too much whether the actual limit size is 4k or 2k. It still allows us to have very deep call stack, and a factor of 2 in the size will not make a real difference. At one point I was looking at which functions have massive stack frames and tried avoiding such state. Now this last set of patches was what I had in my local branch for a long time and decided to finally get rid of them. As you can see, there were multiple cases of 2k buffers being stack allocated, so the end result IMO made sense. The decreasing of the actual limit to 2k isn't as important as I've noted but similarly to when we add a syntax check after a full-repo cleanup it is a way to prevent regressions after a cleanup.
On Tue, Sep 05, 2023 at 09:43:47AM +0200, Peter Krempa wrote:
On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote:
On 8/30/23 13:59, Peter Krempa wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( )
# sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
# array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really understand why this is needed. I mean, we certainly do not want large frames, but IIUC only frames larger than a page are problem (i.e. 4KiB). Can you please point me to some docs where I can learn more?
The main idea of this is to ensure that we don't have massive stack frames which could cause a stack overflow.
But to be honest, it doesn't matter too much whether the actual limit size is 4k or 2k. It still allows us to have very deep call stack, and a factor of 2 in the size will not make a real difference.
At one point I was looking at which functions have massive stack frames and tried avoiding such state. Now this last set of patches was what I had in my local branch for a long time and decided to finally get rid of them. As you can see, there were multiple cases of 2k buffers being stack allocated, so the end result IMO made sense.
The decreasing of the actual limit to 2k isn't as important as I've noted but similarly to when we add a syntax check after a full-repo cleanup it is a way to prevent regressions after a cleanup.
Yeah, given where we've got to with reduced stack size any problems at this point can be safely blamed on something else. eg the language binding that it calling into libvirt - you can quickly get some crazy deep stacks in python for example With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 9/5/23 09:43, Peter Krempa wrote:
On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote:
On 8/30/23 13:59, Peter Krempa wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( )
# sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
# array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really understand why this is needed. I mean, we certainly do not want large frames, but IIUC only frames larger than a page are problem (i.e. 4KiB). Can you please point me to some docs where I can learn more?
The main idea of this is to ensure that we don't have massive stack frames which could cause a stack overflow.
I'm failing to see what stack size and buffer overflow have in common. To overwrite the return address you can has as small buffer as 1 byte and not check the boundaries (obviously). And yet, you can have 2K buffer on stack but if you check the boundaries when reading into it, no exploit is possible.
But to be honest, it doesn't matter too much whether the actual limit size is 4k or 2k. It still allows us to have very deep call stack, and a factor of 2 in the size will not make a real difference.
Yeah. If we care about the actual stack usage (e.g. like kernel does - it has limit of 8KiB for the whole stack of a kernel thread), then restricting individual functions is not enough since we have recursive functions.
At one point I was looking at which functions have massive stack frames and tried avoiding such state. Now this last set of patches was what I had in my local branch for a long time and decided to finally get rid of them. As you can see, there were multiple cases of 2k buffers being stack allocated, so the end result IMO made sense.
Indeed and this reason alone is good enough. It made the code cleaner too. I was just wondering whether there is some deeper sense.
The decreasing of the actual limit to 2k isn't as important as I've noted but similarly to when we add a syntax check after a full-repo cleanup it is a way to prevent regressions after a cleanup.
BTW: if you (or anybody else) want to identify functions which have massive stack usage, I've found clever perl script from kernel.git: libvirt.git $ objdump -d _build/src/libvirt.so.0.9008.0 | \ kernel.git/scripts/checkstack.pl | \ head -n30 some very interesting functions appear on the list (virDomainDefFeaturesCheckABIStability()) - but when looking at the disassembled code directly, it's not just about initial sub 0x68, %rsp; it's also about how many arguments are passed to functions (esp. with variable arguments) Michal
On Tue, Sep 05, 2023 at 11:05:17 +0200, Michal Prívozník wrote:
On 9/5/23 09:43, Peter Krempa wrote:
On Mon, Sep 04, 2023 at 16:09:00 +0200, Michal Prívozník wrote:
On 8/30/23 13:59, Peter Krempa wrote:
After recent cleanups we can now restrict the maximum stack frame size to 2k.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build index 965ada483b..e45f3e2c39 100644 --- a/meson.build +++ b/meson.build @@ -248,7 +248,7 @@ alloc_max = run_command( )
# sanitizer instrumentation may enlarge stack frames -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768 +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
# array_bounds=2 check triggers false positive on some GCC # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really understand why this is needed. I mean, we certainly do not want large frames, but IIUC only frames larger than a page are problem (i.e. 4KiB). Can you please point me to some docs where I can learn more?
The main idea of this is to ensure that we don't have massive stack frames which could cause a stack overflow.
I'm failing to see what stack size and buffer overflow have in common.
I've specifically said "stack overflow". The issue where you put so much stuff on the stack that you run out of stack space.
To overwrite the return address you can has as small buffer as 1 byte and not check the boundaries (obviously). And yet, you can have 2K buffer on stack but if you check the boundaries when reading into it, no exploit is possible.
But to be honest, it doesn't matter too much whether the actual limit size is 4k or 2k. It still allows us to have very deep call stack, and a factor of 2 in the size will not make a real difference.
Yeah. If we care about the actual stack usage (e.g. like kernel does - it has limit of 8KiB for the whole stack of a kernel thread), then restricting individual functions is not enough since we have recursive functions.
Normal programs have 8MiB per stack. We do have recursive functions but by limiting the stack size to 2k now we can get very deep. That is the reason I've also said that it really doesn't matter that much whether it's 4k or 2k. It's just a factor of 2 improvement.
At one point I was looking at which functions have massive stack frames and tried avoiding such state. Now this last set of patches was what I had in my local branch for a long time and decided to finally get rid of them. As you can see, there were multiple cases of 2k buffers being stack allocated, so the end result IMO made sense.
Indeed and this reason alone is good enough. It made the code cleaner too. I was just wondering whether there is some deeper sense.
The decreasing of the actual limit to 2k isn't as important as I've noted but similarly to when we add a syntax check after a full-repo cleanup it is a way to prevent regressions after a cleanup.
BTW: if you (or anybody else) want to identify functions which have massive stack usage, I've found clever perl script from kernel.git:
libvirt.git $ objdump -d _build/src/libvirt.so.0.9008.0 | \ kernel.git/scripts/checkstack.pl | \ head -n30
some very interesting functions appear on the list (virDomainDefFeaturesCheckABIStability()) - but when looking at the disassembled code directly, it's not just about initial sub 0x68, %rsp; it's also about how many arguments are passed to functions (esp. with variable arguments)
Michal
participants (4)
-
Daniel P. Berrangé -
Kristina Hanicova -
Michal Prívozník -
Peter Krempa