Message ID | d6cf1205-d537-fafb-a082-e973bfe11315@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [4.15] x86/shadow: replace bogus return path in shadow_get_page_from_l1e() | expand |
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
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>
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>
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.
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
--- 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);
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>