Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
error code") a positive return value did indicate an error. Said commit
failed to adjust this return path, but luckily the only caller has
always been inside a shadow_mode_refcounts() conditional.

Subsequent changes caused 1 to end up at the default (error) label in
the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
is still rather confusing here, and a latent risk.

Convert to an ASSERT() instead, just in case any new caller would
appear.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -802,9 +802,7 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
     struct domain *owner;
 
     ASSERT(!sh_l1e_is_magic(sl1e));
-
-    if ( !shadow_mode_refcounts(d) )
-        return 1;
+    ASSERT(shadow_mode_refcounts(d));
 
     res = get_page_from_l1e(sl1e, d, d);
 

On 26/02/2021 15:08, Jan Beulich wrote:
> Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
> error code") a positive return value did indicate an error. Said commit
> failed to adjust this return path, but luckily the only caller has
> always been inside a shadow_mode_refcounts() conditional.
>
> Subsequent changes caused 1 to end up at the default (error) label in
> the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
> is still rather confusing here, and a latent risk.
>
> Convert to an ASSERT() instead, just in case any new caller would
> appear.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Yikes, and only 9 years to notice.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Andrew Cooper writes ("Re: [PATCH][4.15] x86/shadow: replace bogus return path in shadow_get_page_from_l1e()"):
> On 26/02/2021 15:08, Jan Beulich wrote:
> > Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
> > error code") a positive return value did indicate an error. Said commit
> > failed to adjust this return path, but luckily the only caller has
> > always been inside a shadow_mode_refcounts() conditional.
> >
> > Subsequent changes caused 1 to end up at the default (error) label in
> > the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
> > is still rather confusing here, and a latent risk.
> >
> > Convert to an ASSERT() instead, just in case any new caller would
> > appear.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Yikes, and only 9 years to notice.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Ah here we are,

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks

At 16:08 +0100 on 26 Feb (1614355713), Jan Beulich wrote:
> Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
> error code") a positive return value did indicate an error. Said commit
> failed to adjust this return path, but luckily the only caller has
> always been inside a shadow_mode_refcounts() conditional.
> 
> Subsequent changes caused 1 to end up at the default (error) label in
> the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
> is still rather confusing here, and a latent risk.
> 
> Convert to an ASSERT() instead, just in case any new caller would
> appear.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

On 26.02.2021 16:08, Jan Beulich wrote:
> Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
> error code") a positive return value did indicate an error. Said commit
> failed to adjust this return path, but luckily the only caller has
> always been inside a shadow_mode_refcounts() conditional.
> 
> Subsequent changes caused 1 to end up at the default (error) label in
> the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
> is still rather confusing here, and a latent risk.

The confusion on my part was so significant that I screwed up
the shadow mode fix for "VMX: use a single, global APIC access
page" (which turned out to be necessary) initially. Hence my
proposing this for 4.15. I'm on the edge at this point whether
I'd even consider this a backporting candidate.

Jan

Jan Beulich writes ("Re: [PATCH][4.15] x86/shadow: replace bogus return path in shadow_get_page_from_l1e()"):
> On 26.02.2021 16:08, Jan Beulich wrote:
> > Prior to be640b1800bb ("x86: make get_page_from_l1e() return a proper
> > error code") a positive return value did indicate an error. Said commit
> > failed to adjust this return path, but luckily the only caller has
> > always been inside a shadow_mode_refcounts() conditional.
> > 
> > Subsequent changes caused 1 to end up at the default (error) label in
> > the caller's switch() again, but the returning of 1 (== _PAGE_PRESENT)
> > is still rather confusing here, and a latent risk.
> 
> The confusion on my part was so significant that I screwed up
> the shadow mode fix for "VMX: use a single, global APIC access
> page" (which turned out to be necessary) initially. Hence my
> proposing this for 4.15.

Right.  I'm sympathetic, but I would like to hear from another
maintainer of this code, as to their opinion about how much this
change removes the potential for confusion.

>  I'm on the edge at this point whether
> I'd even consider this a backporting candidate.

I think you mean you think that even though the code does not compile
to something actually buggy now, this is sufficiently bad a confusion
that it risks bugs in stable trees, so you are considering
backporting.

Thanks, that is a useful piece of perspective.

Ian.