[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v9 02/11] xen/domain: Add vmtrace_size domain creation parameter


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 3 Feb 2021 16:04:26 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=lO+KDfaUir45msS+7YB6vlvOLGHOXRxWbwqCsjofHBI=; b=K1quyAiQPoKRzyQTqCxNf/2vCYnEpKJaENNG6DG7ACUQnBlqnTBKB3H9xCkN27OfSVXmR0AttzubrcpYQD2kpIdiuNQrbebmdupaoQJtlH61AFBx9BeoLmFMeqXZLS4uvxo3zQvMAm2MVPP2U6tYoExqpIRmovTZKOEK1mZncuv4niebn/757IWunxYAkDHdZz5uV/XmxuRngfQnRUs3F8cv6ONJFJaDhTCvJU7Wcsa+/T4aNys1UdiB2g1wl7b5QMJp+/E6fPCT2SuzanaOYHVEfUFnb/LKn/hG7xDEXclDrip6vdgJfPX1XDeRkTyZ08leTpSM23FK9yOsvQBFtQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mNkHpSWjkdNixoav69+gmC5t+ehHL4MFV0feZedkiR+HU1q6UQo5LYZbpmMuw/sYWXYyD2mTSpSK3lVZOmDsTUjlhO1MaBRT5GzjQxSV1AZGKvlZOook/8e3yMvTyij82b9X7N8V0BKJIa943fTRVXTr+8xHoEEEHFBSwudPDV/avp/22OFzJ5UJLO0mehFMKfi+BlB9C7q1MSit4QR6wrMImHIfele0HHml4pYNqMd0bthwRT7e3wDKJVXxnkXHtrPmQWeAVUoQer1mEeHVcScwhifKxak16PY3vn0HAe5fwqk7y/50PhYqo6FS96SZHzr3rmLcdEQ6kLB8NjC9ZQ==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Michał Leszczyński <michal.leszczynski@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 03 Feb 2021 16:04:48 +0000
  • Ironport-sdr: defHSGZdOoBab3I5xd7Y1qr6iEjMatyV/109R3K1Rq2KZlsQiM81fQGT5QaFXIWyN6BLhk2wSd e5vHGZmCp5nEYFNW92XGf2Z0dawrbyjYTjso1djNwzChfZ632cgrTC2cTp8kMQA8HuAnCen14T Ay1OnYRxH95OE7/pQ6euQI+ihesxIKf6giXwj/azVsx+0F8056CCYj2ec0PxD0Xx5xzDGsHZi3 JNHTHSUxRJuz9kaug6YaYLhVRv1YPLSMI0CFx8HTJdtH0f0UDeqopB5x1w03WXWF+Vy1J9gcoT DkI=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 02/02/2021 09:04, Jan Beulich wrote:
> On 02.02.2021 00:26, Andrew Cooper wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -132,6 +132,56 @@ static void vcpu_info_reset(struct vcpu *v)
>>      v->vcpu_info_mfn = INVALID_MFN;
>>  }
>>  
>> +static void vmtrace_free_buffer(struct vcpu *v)
>> +{
>> +    const struct domain *d = v->domain;
>> +    struct page_info *pg = v->vmtrace.pg;
>> +    unsigned int i;
>> +
>> +    if ( !pg )
>> +        return;
>> +
>> +    v->vmtrace.pg = NULL;
>> +
>> +    for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> +    {
>> +        put_page_alloc_ref(&pg[i]);
>> +        put_page_and_type(&pg[i]);
>> +    }
>> +}
>> +
>> +static int vmtrace_alloc_buffer(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +    struct page_info *pg;
>> +    unsigned int i;
>> +
>> +    if ( !d->vmtrace_size )
>> +        return 0;
>> +
>> +    pg = alloc_domheap_pages(d, get_order_from_bytes(d->vmtrace_size),
>> +                             MEMF_no_refcount);
>> +    if ( !pg )
>> +        return -ENOMEM;
>> +
>> +    for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
>> +        if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
>> +            goto refcnt_err;
>> +
>> +    /*
>> +     * We must only let vmtrace_free_buffer() take any action in the success
>> +     * case when we've taken all the refs it intends to drop.
>> +     */
>> +    v->vmtrace.pg = pg;
>> +    return 0;
>> +
>> + refcnt_err:
>> +    while ( i-- )
>> +        put_page_and_type(&pg[i]);
>> +
>> +    return -ENODATA;
> Would you mind at least logging how many pages may be leaked
> here? I also don't understand why you don't call
> put_page_alloc_ref() in the loop - that's fine to do prior to
> the put_page_and_type(), and will at least limit the leak.
> The buffer size here typically isn't insignificant, and it
> may be helpful to not unnecessarily defer the freeing to
> relinquish_resources() (assuming we will make that one also
> traverse the list of "extra" pages, but I understand that's
> not going to happen for 4.15 anymore anyway).
>
> Additionally, while I understand you're not in favor of the
> comments we have on all three similar code paths, I think
> replicating their comments here would help easily spotting
> (by grep-ing e.g. for "fishy") all instances that will need
> adjusting once we have figured a better overall solution.

How is:

    for ( i = 0; i < (d->vmtrace_size >> PAGE_SHIFT); i++ )
        if ( unlikely(!get_page_and_type(&pg[i], d, PGT_writable_page)) )
            /*
             * The domain can't possibly know about this page yet, so
failure
             * here is a clear indication of something fishy going on.
             */
            goto refcnt_err;

    /*
     * We must only let vmtrace_free_buffer() take any action in the success
     * case when we've taken all the refs it intends to drop.
     */
    v->vmtrace.pg = pg;
    return 0;

 refcnt_err:
    /*
     * We can theoretically reach this point if someone has taken 2^43
refs on
     * the frames in the time the above loop takes to execute, or
someone has
     * made a blind decrease reservation hypercall and managed to pick the
     * right mfn.  Free the memory we safely can, and leak the rest.
     */
    while ( i-- )
    {
        put_page_alloc_ref(&pg[i]);
        put_page_and_type(&pg[i]);
    }

    return -ENODATA;

this?

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.