Re: [PATCH v2 2/6] x86/mem-paging: correct p2m_mem_paging_prep()'s error handling

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2 2/6] x86/mem-paging: correct p2m_mem_paging_prep()'s error handling
Date: Fri, 15 May 2020 21:02:38 +0100	[thread overview]
Message-ID: <1e390685-dbca-afee-0cda-692b1134183e@citrix.com> (raw)
In-Reply-To: <83f6463c-6a61-e79b-cf1b-77589ef287c1@suse.com>

On 15/05/2020 16:15, Jan Beulich wrote:
>>> +            domain_crash(d);
> This already leaves a file/line combination as a (minimal hint).

First, that is still tantamount to useless in logs from a user.

Second, the use of __LINE__ is why it breaks livepatching, and people
using livepatching is still carrying an out-of-tree patch to unbreak it.

> I can make a patch to add a gprintk() as you ask for, but I'm not
> sure it's worth it for this almost dead code.

"page in unexpected state" would be better than nothing, but given the
comment, it might also be better as ASSERT_UNREACHABLE(), and we now
have a lot of cases where we declare unreachable, and kill the domain in
release builds.

>
>>> @@ -1843,13 +1852,24 @@ int p2m_mem_paging_prep(struct domain *d
>>>      ret = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
>>>                          paging_mode_log_dirty(d) ? p2m_ram_logdirty
>>>                                                   : p2m_ram_rw, a);
>>> -    set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
>>> +    if ( !ret )
>>> +    {
>>> +        set_gpfn_from_mfn(mfn_x(mfn), gfn_l);
>>>  
>>> -    if ( !page_extant )
>>> -        atomic_dec(&d->paged_pages);
>>> +        if ( !page_extant )
>>> +            atomic_dec(&d->paged_pages);
>>> +    }
>>>  
>>>   out:
>>>      gfn_unlock(p2m, gfn, 0);
>>> +
>>> +    if ( page )
>>> +    {
>>> +        if ( ret )
>>> +            put_page_alloc_ref(page);
>>> +        put_page(page);
>> This is a very long way from clear enough to follow, and buggy if anyone
>> inserts a new goto out path.
> What alternatives do you see?

/* Fully free the page on error.  Drop our temporary reference in all
cases. */

would at least help someone trying to figure out what is going on here,
especially as put_page_alloc_ref() is not the obvious freeing function
for alloc_domheap_page().

~Andrew