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

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 17/11/2022 10:39, Jan Beulich wrote:
> 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@citrix.com>
>> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> (if this counts anything, since as it stands the new stuff all falls
> under tool stack maintainership)

I do intend to give it its own section in due course, but this falls
very much into "The Rest" at the moment.

>> --- /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.

Checking capabilities needs to happen anyway to get PV and HVM Shadow
support working.

But this will do for now.

>> +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.

Mechanically, no.  x86 PV is an entirely arbitrary mapping, while all
HVM modes (x86 and ARM) are just filling in a guest physical -> "some
RAM" mapping in a pagetable structure.

Logically, I hope that CDF_directmap on ARM checks for an alias to a
real RAM block, but this capability isn't even exposed to the toolstack.


And now I've looked at this, ewwww.  We've got:

d->options which holds config->flags, DOMCTL_CDF_*
d->cdf which holds the local 'flags' parameter, CFD_*
d->is_privileged which holds 'flags & CDF_privileged' and was never
cleaned up when d->cdf was introduced.

This is unnecessarily confusing to follow.  Yet more for the cleanup pile.

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

Fixed.

>
>> +     * 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)?

Hmm yeah.  That was rather stale.

I've switched to "Test set continuation\n" seeing as that's the purpose
of the test.  64M is an internal detail which ought to be small enough
to work reliably everywhere, but large enough to cause a continuation.

~Andrew