On Mon, Apr 23, 2018 at 05:44:38PM +0200, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1569678

On some large systems (with ~400GB of RAM) it is possible for unsigned int to overflow in which case we report invalid number of 4K pages pool size. Switch to unsigned long long.

It isn't very obvious from the code diff what the actual problem is, but it is mentioned in the bug that we hit overflow in virNumaGetPages when doing: huge_page_sum += 1024 * page_size * page_avail; because although 'huge_page_sum' is an unsigned long long, the page_size and page_avail are both unsigned int, so the promotion to unsigned long long doesn't happen until the sum has been calculated, by which time we've already overflowed. Can you mention that we're specifically solving this huge_page_sum overflow in the commit message Turning page_avail into a unsigned long long is not strictly needed until we need ability to represent more than 2^32 4k pages, which IIUC equates to 16 TB of RAM. That's not outside the realm of possibility, so makes sense that we change it to unsigned long long to avoid future problems. Can you also mention that we're solving this limit too in the commit message. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 5 +++-- src/conf/capabilities.h | 2 +- src/util/virhostmem.c | 2 +- src/util/virnuma.c | 32 ++++++++++++++++++-------------- src/util/virnuma.h | 8 ++++---- tests/virnumamock.c | 4 ++-- 6 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index c4ee7efb5f..dd2fc77f91 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -816,7 +816,7 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf, cells[i]->mem);

for (j = 0; j < cells[i]->npageinfo; j++) { - virBufferAsprintf(buf, "<pages unit='KiB' size='%u'>%zu</pages>\n", + virBufferAsprintf(buf, "<pages unit='KiB' size='%u'>%llu</pages>\n", cells[i]->pageinfo[j].size, cells[i]->pageinfo[j].avail); } @@ -1351,7 +1351,8 @@ virCapabilitiesGetNUMAPagesInfo(int node, int *npageinfo) { int ret = -1; - unsigned int *pages_size = NULL, *pages_avail = NULL; + unsigned int *pages_size = NULL; + unsigned long long *pages_avail = NULL; size_t npages, i;

if (virNumaGetPages(node, &pages_size, &pages_avail, NULL, &npages) < 0) diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 694a3590bf..f0a06a24df 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -107,7 +107,7 @@ typedef struct _virCapsHostNUMACellPageInfo virCapsHostNUMACellPageInfo; typedef virCapsHostNUMACellPageInfo *virCapsHostNUMACellPageInfoPtr; struct _virCapsHostNUMACellPageInfo { unsigned int size; /* page size in kibibytes */ - size_t avail; /* the size of pool */ + unsigned long long avail; /* the size of pool */ };

typedef struct _virCapsHostNUMACell virCapsHostNUMACell; diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index 11efe8c502..c923a1edf5 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -783,7 +783,7 @@ virHostMemGetFreePages(unsigned int npages, for (cell = startCell; cell <= lastCell; cell++) { for (i = 0; i < npages; i++) { unsigned int page_size = pages[i]; - unsigned int page_free; + unsigned long long page_free;

if (virNumaGetPageInfo(cell, page_size, 0, NULL, &page_free) < 0) goto cleanup; diff --git a/src/util/virnuma.c b/src/util/virnuma.c index bebe301f8d..784db0a7ce 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -563,8 +563,8 @@ virNumaGetHugePageInfoDir(char **path, int node) static int virNumaGetHugePageInfo(int node, unsigned int page_size, - unsigned int *page_avail, - unsigned int *page_free) + unsigned long long *page_avail, + unsigned long long *page_free) { int ret = -1; char *path = NULL; @@ -579,7 +579,7 @@ virNumaGetHugePageInfo(int node, if (virFileReadAll(path, 1024, &buf) < 0) goto cleanup;

- if (virStrToLong_ui(buf, &end, 10, page_avail) < 0 || + if (virStrToLong_ull(buf, &end, 10, page_avail) < 0 || *end != '\n') { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse: %s"), @@ -598,7 +598,7 @@ virNumaGetHugePageInfo(int node, if (virFileReadAll(path, 1024, &buf) < 0) goto cleanup;

- if (virStrToLong_ui(buf, &end, 10, page_free) < 0 || + if (virStrToLong_ull(buf, &end, 10, page_free) < 0 || *end != '\n') { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse: %s"), @@ -645,8 +645,8 @@ int virNumaGetPageInfo(int node, unsigned int page_size, unsigned long long huge_page_sum, - unsigned int *page_avail, - unsigned int *page_free) + unsigned long long *page_avail, + unsigned long long *page_free) { int ret = -1; long system_page_size = virGetSystemPageSize(); @@ -709,8 +709,8 @@ virNumaGetPageInfo(int node, int virNumaGetPages(int node, unsigned int **pages_size, - unsigned int **pages_avail, - unsigned int **pages_free, + unsigned long long **pages_avail, + unsigned long long **pages_free, size_t *npages) { int ret = -1; @@ -718,7 +718,9 @@ virNumaGetPages(int node, DIR *dir = NULL; int direrr = 0; struct dirent *entry; - unsigned int *tmp_size = NULL, *tmp_avail = NULL, *tmp_free = NULL; + unsigned int *tmp_size = NULL; + unsigned long long *tmp_avail = NULL; + unsigned long long *tmp_free = NULL; unsigned int ntmp = 0; size_t i; bool exchange; @@ -744,7 +746,9 @@ virNumaGetPages(int node,

while (dir && (direrr = virDirRead(dir, &entry, path)) > 0) { const char *page_name = entry->d_name; - unsigned int page_size, page_avail = 0, page_free = 0; + unsigned int page_size; + unsigned long long page_avail = 0; + unsigned long long page_free = 0; char *end;

/* Just to give you a hint, we're dealing with this: @@ -934,8 +938,8 @@ int virNumaGetPageInfo(int node ATTRIBUTE_UNUSED, unsigned int page_size ATTRIBUTE_UNUSED, unsigned long long huge_page_sum ATTRIBUTE_UNUSED, - unsigned int *page_avail ATTRIBUTE_UNUSED, - unsigned int *page_free ATTRIBUTE_UNUSED) + unsigned long long *page_avail ATTRIBUTE_UNUSED, + unsigned long long *page_free ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("page info is not supported on this platform")); @@ -946,8 +950,8 @@ virNumaGetPageInfo(int node ATTRIBUTE_UNUSED, int virNumaGetPages(int node ATTRIBUTE_UNUSED, unsigned int **pages_size ATTRIBUTE_UNUSED, - unsigned int **pages_avail ATTRIBUTE_UNUSED, - unsigned int **pages_free ATTRIBUTE_UNUSED, + unsigned long long **pages_avail ATTRIBUTE_UNUSED, + unsigned long long **pages_free ATTRIBUTE_UNUSED, size_t *npages ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", diff --git a/src/util/virnuma.h b/src/util/virnuma.h index e4e1fd0b97..a3ffb6d6c7 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -52,12 +52,12 @@ int virNumaGetNodeCPUs(int node, virBitmapPtr *cpus) ATTRIBUTE_NOINLINE; int virNumaGetPageInfo(int node, unsigned int page_size, unsigned long long huge_page_sum, - unsigned int *page_avail, - unsigned int *page_free); + unsigned long long *page_avail, + unsigned long long *page_free); int virNumaGetPages(int node, unsigned int **pages_size, - unsigned int **pages_avail, - unsigned int **pages_free, + unsigned long long **pages_avail, + unsigned long long **pages_free, size_t *npages) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NOINLINE; int virNumaSetPagePoolSize(int node, diff --git a/tests/virnumamock.c b/tests/virnumamock.c index d8f90b81b3..475efc1f34 100644 --- a/tests/virnumamock.c +++ b/tests/virnumamock.c @@ -125,8 +125,8 @@ virNumaGetDistances(int node ATTRIBUTE_UNUSED, int virNumaGetPages(int node, unsigned int **pages_size, - unsigned int **pages_avail, - unsigned int **pages_free, + unsigned long long **pages_avail, + unsigned long long **pages_free, size_t *npages) { const int pages_def[] = { 4, 2 * 1024, 1 * 1024 * 1024};

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 :|