One of the error paths in the PV dom0 builder section that runs on the guest
page-tables wasn't restoring the Xen value of %cr3, neither removing the
mapcache override.

Fixes: 079ff2d32c3d ('libelf-loader: introduce elf_load_image')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/dom0_build.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index d8043fa58a27..57e58a02e707 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -825,6 +825,8 @@ int __init dom0_construct_pv(struct domain *d,
     rc = elf_load_binary(&elf);
     if ( rc < 0 )
     {
+        mapcache_override_current(NULL);
+        switch_cr3_cr4(current->arch.cr3, read_cr4());
         printk("Failed to load the kernel binary\n");
         goto out;
     }
-- 
2.45.2

On 30.07.2024 17:28, Roger Pau Monne wrote:
> One of the error paths in the PV dom0 builder section that runs on the guest
> page-tables wasn't restoring the Xen value of %cr3, neither removing the
> mapcache override.

s/neither/nor/ ?

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -825,6 +825,8 @@ int __init dom0_construct_pv(struct domain *d,
>      rc = elf_load_binary(&elf);
>      if ( rc < 0 )
>      {
> +        mapcache_override_current(NULL);
> +        switch_cr3_cr4(current->arch.cr3, read_cr4());
>          printk("Failed to load the kernel binary\n");
>          goto out;
>      }

Just below here we have

    bootstrap_map(NULL);

This too is wanted in the error case aiui. Happy to move it up immediately
ahead of the if() while committing, so long as you agree. Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On Wed, Jul 31, 2024 at 08:32:03AM +0200, Jan Beulich wrote:
> On 30.07.2024 17:28, Roger Pau Monne wrote:
> > One of the error paths in the PV dom0 builder section that runs on the guest
> > page-tables wasn't restoring the Xen value of %cr3, neither removing the
> > mapcache override.
> 
> s/neither/nor/ ?
> 
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -825,6 +825,8 @@ int __init dom0_construct_pv(struct domain *d,
> >      rc = elf_load_binary(&elf);
> >      if ( rc < 0 )
> >      {
> > +        mapcache_override_current(NULL);
> > +        switch_cr3_cr4(current->arch.cr3, read_cr4());
> >          printk("Failed to load the kernel binary\n");
> >          goto out;
> >      }
> 
> Just below here we have
> 
>     bootstrap_map(NULL);
> 
> This too is wanted in the error case aiui. Happy to move it up immediately
> ahead of the if() while committing, so long as you agree. Then:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

I'm happy with this, but note there are further instances of error
paths above this one that already don't remove the bootstrap mappings.

Thanks, Roger.

On 31.07.2024 09:57, Roger Pau Monné wrote:
> On Wed, Jul 31, 2024 at 08:32:03AM +0200, Jan Beulich wrote:
>> On 30.07.2024 17:28, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>> @@ -825,6 +825,8 @@ int __init dom0_construct_pv(struct domain *d,
>>>      rc = elf_load_binary(&elf);
>>>      if ( rc < 0 )
>>>      {
>>> +        mapcache_override_current(NULL);
>>> +        switch_cr3_cr4(current->arch.cr3, read_cr4());
>>>          printk("Failed to load the kernel binary\n");
>>>          goto out;
>>>      }
>>
>> Just below here we have
>>
>>     bootstrap_map(NULL);
>>
>> This too is wanted in the error case aiui. Happy to move it up immediately
>> ahead of the if() while committing, so long as you agree. Then:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm happy with this, but note there are further instances of error
> paths above this one that already don't remove the bootstrap mappings.

Hmm, you're right. I'll leave that untouched then.

Jan