Re: [PATCH 2/4] tools/tests: Unit test for paging mempool size

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.11.2022 02:08, Andrew Cooper wrote:
> Exercise some basic functionality of the new
> xc_{get,set}_paging_mempool_size() hypercalls.
> 
> This passes on x86, but fails currently on ARM.  ARM will be fixed up in
> future patches.
> 
> This is part of XSA-409 / CVE-2022-33747.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
(if this counts anything, since as it stands the new stuff all falls
under tool stack maintainership)

> --- /dev/null
> +++ b/tools/tests/paging-mempool/test-paging-mempool.c
> @@ -0,0 +1,181 @@
> +#include <err.h>
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +
> +#include <xenctrl.h>
> +#include <xenforeignmemory.h>
> +#include <xengnttab.h>
> +#include <xen-tools/libs.h>
> +
> +static unsigned int nr_failures;
> +#define fail(fmt, ...)                          \
> +({                                              \
> +    nr_failures++;                              \
> +    (void)printf(fmt, ##__VA_ARGS__);           \
> +})
> +
> +static xc_interface *xch;
> +static uint32_t domid;
> +
> +static struct xen_domctl_createdomain create = {
> +    .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,

I understand that it is accepted that this test will thus fail when run
on HAP-incapable hardware (including when run with Xen itself running on
top of another hypervisor not surfacing HAP capabilities)? Oh, I notice
you're actually translating EINVAL and EOPNOTSUPP failures into "skip".
That'll probably do, albeit personally I consider skipping when EINVAL
(which we use all over the place) as a overly relaxed.

> +static void run_tests(void)
> +{
> +    xen_pfn_t physmap[] = { 0 };

I have to admit that I'm uncertain whether Arm (or other architectures
that Xen is being planned to be ported to) have constraints which may
cause populating of GFN 0 to fail.

> +    uint64_t size_bytes, old_size_bytes;
> +    int rc;
> +
> +    printf("Test default mempool size\n");
> +
> +    rc = xc_get_paging_mempool_size(xch, domid, &size_bytes);
> +    if ( rc )
> +        return fail("  Fail: get mempool size: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    printf("mempool size %"PRIu64" bytes (%"PRIu64"kB, %"PRIu64"MB)\n",
> +           size_bytes, size_bytes >> 10, size_bytes >> 20);
> +
> +
> +    /*
> +     * Check that the domain has the expected default allocation size.  This
> +     * will fail if the logic in Xen is altered without an equivelent

Nit: equivalent

> +     * adjustment here.
> +     */
> +    if ( size_bytes != default_mempool_size_bytes )
> +        return fail("  Fail: size %"PRIu64" != expected size %"PRIu64"\n",
> +                    size_bytes, default_mempool_size_bytes);
> +
> +
> +    printf("Test that allocate doesn't alter pool size\n");
> +
> +    /*
> +     * Populate the domain with some RAM.  This will cause more of the 
> mempool
> +     * to be used.
> +     */
> +    old_size_bytes = size_bytes;
> +
> +    rc = xc_domain_setmaxmem(xch, domid, -1);
> +    if ( rc )
> +        return fail("  Fail: setmaxmem: : %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    rc = xc_domain_populate_physmap_exact(xch, domid, 1, 0, 0, physmap);
> +    if ( rc )
> +        return fail("  Fail: populate physmap: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    /*
> +     * Re-get the p2m size.  Should not have changed as a consequence of
> +     * populate physmap.
> +     */
> +    rc = xc_get_paging_mempool_size(xch, domid, &size_bytes);
> +    if ( rc )
> +        return fail("  Fail: get mempool size: %d - %s\n",
> +                    errno, strerror(errno));
> +
> +    if ( old_size_bytes != size_bytes )
> +        return fail("  Fail: mempool size changed %"PRIu64" => %"PRIu64"\n",
> +                    old_size_bytes, size_bytes);
> +
> +
> +
> +    printf("Test bad set size\n");
> +
> +    /*
> +     * Check that setting a non-page size results in failure.
> +     */
> +    rc = xc_set_paging_mempool_size(xch, domid, size_bytes + 1);
> +    if ( rc != -1 || errno != EINVAL )
> +        return fail("  Fail: Bad set size: expected -1/EINVAL, got %d/%d - 
> %s\n",
> +                    rc, errno, strerror(errno));
> +
> +
> +    printf("Test very large set size\n");

Maybe drop "very", as 64M isn't all that much (and would, in particular,
not expose any 32-bit truncation issues)?

Jan