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

Re: [PATCH] common: guest_physmap_add_page()'s return value needs checking


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 21 Sep 2021 12:28:12 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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; bh=+yRVntTgbnnOC9d18dQVZrXN5kZgOLcXWkhZgnh+hls=; b=WdTve8cjiMzgWC9qy4FYtY1d5DDjhhGooJg1keaIlde7vv5XvXzb2XAHRMAVJW20zrfhbKgKi2GaFO1g6MenhMxkHln8qEe4a/XUbGVTkaCuQ3ZNbkyqNFR56VOA1TkIFFRWCmDii0mw/1oJkuqkBHl1NkjGQWwNxvJXQ5m0UDFoQ/w9aHQh0X+R6CxO/i2e+t83e0f6F9/SJEcZu+M+idXstCOAeeTrbkb1jPPvj9bjcJfboHPs9NUuCulxKVonrA1KJkh/WWM9AuC9jKXRLkIZjQtQcSr4VLS5vIepCx6mVYtGBvT86fU1APRE4bX/ZuTPRyqBV0rVUBQQKh8iJQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Kk/ly3Mo+OU9uTEiDUTHO51wtKmTYlwOJvBUCT8JSJk43kvvF7aWh5sdTUjIVePW9yLJNRQV/D0/xhHioXUmAXcFETMdH6CBuFVpgh6Qcr9NRg8LRtK/qa259XF59DA9w5ip6fK5FphEIPNEMLogKGBMaOJJqwzRT2ZVnwDjxsfe1ZkLSfRwvPKEMWeXEf/M/+72DT3B/cZ99lVklWZs9QJrShYxQdvto2RDOys8eYdUZIibPrauvvWCLj6hxxfU6i1XQpOY10IyHYzqn/oyIrXmqFpbIew1X/8XKr5r0r+pAbrrCzROw/IAGjPcpRd18WTrIq5PJEBzrenqd20l0g==
  • Authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Tue, 21 Sep 2021 10:28:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 21.09.2021 11:20, Roger Pau Monné wrote:
> On Wed, Sep 01, 2021 at 06:06:37PM +0200, Jan Beulich wrote:
>> The function may fail; it is not correct to indicate "success" in this
>> case up the call stack. Mark the function must-check to prove all
>> cases have been caught (and no new ones will get introduced).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks. Albeit strictly speaking an ack here isn't enough for the change
to go in, it would need to be R-b or come from a REST maintainer.

>> ---
>> In the grant-transfer case it is not really clear to me whether we can
>> stick to setting GTF_transfer_completed in the error case. Since a guest
>> may spin-wait for the flag to become set, simply not setting the flag is
>> not an option either. I was wondering whether we may want to slightly
>> alter (extend) the ABI and allow for a GTF_transfer_committed ->
>> GTF_transfer_completed transition (i.e. clearing GTF_transfer_committed
>> at the same time as setting GTF_transfer_completed).
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2394,7 +2394,7 @@ gnttab_transfer(
>>          {
>>              grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, 
>> gop.ref);
>>  
>> -            guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
>> +            rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
>>              if ( !paging_mode_translate(e) )
>>                  sha->frame = mfn_x(mfn);
>>          }
>> @@ -2402,7 +2402,7 @@ gnttab_transfer(
>>          {
>>              grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, 
>> gop.ref);
>>  
>> -            guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 0);
>> +            rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 
>> 0);
>>              if ( !paging_mode_translate(e) )
>>                  sha->full_page.frame = mfn_x(mfn);
> 
> Is it fine to set the frame even if updating the physmap failed?

Well - the page is now owned by that domain, so there's nothing outright
wrong with reporting its MFN. guest_physmap_add_page() failing in the
!paging_mode_translate() is also only possible under obscure conditions,
with the guest guessing about MFNs it is in the process of getting
assigned.

>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -268,7 +268,8 @@ static void populate_physmap(struct memo
>>                  mfn = page_to_mfn(page);
>>              }
>>  
>> -            guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
>> +            if ( guest_physmap_add_page(d, _gfn(gpfn), mfn, 
>> a->extent_order) )
>> +                goto out;
>>  
>>              if ( !paging_mode_translate(d) &&
>>                   /* Inform the domain of the new page's machine address. */
>> @@ -765,8 +766,8 @@ static long memory_exchange(XEN_GUEST_HA
>>              }
>>  
>>              mfn = page_to_mfn(page);
>> -            guest_physmap_add_page(d, _gfn(gpfn), mfn,
>> -                                   exch.out.extent_order);
>> +            rc = guest_physmap_add_page(d, _gfn(gpfn), mfn,
>> +                                        exch.out.extent_order) ?: rc;
>>              if ( !paging_mode_translate(d) &&
>>                   __copy_mfn_to_guest_offset(exch.out.extent_start,
> 
> Would it be worth it setting the mfn on the guest output to
> INVALID_MFN or some such if the physmap addition failed?

Like above - the page is in possession of the guest now. Once it knows
of the MFN it may be able to do something to remedy the error (at the
very least: free the page again, e.g. via decrease-reservation, where
only the MFN is needed).

Of course in both cases a prereq requirement on guest behavior would
be that they consume the output field in the first place despite the
error, which in turn requires them to prefill the field with a sentinel
allowing them to recognize whether a valid MFN was passed back.

Jan




 


Rackspace

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