LLVM code generation can attempt to load from a variable in the next
condition of an expression under certain circumstances, thus
attempting to load use_xsave regardless of the value of the bsp
variable, which leads to a page fault when the init section has
already been unmapped.

Fix this by making use_xsave non-init, thus preventing the page fault.
The LLVM bug with the discussion about this issue can be found at:

https://bugs.llvm.org/show_bug.cgi?id=39707

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 xen/arch/x86/xstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 3293ef834f..6a7353109b 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
 /* Collect the information of processor's extended state */
 void xstate_init(struct cpuinfo_x86 *c)
 {
-    static bool __initdata use_xsave = true;
+    static bool use_xsave = true;
     boolean_param("xsave", use_xsave);
 
     bool bsp = c == &boot_cpu_data;
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 01.07.2019 12:49, Roger Pau Monne wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>   /* Collect the information of processor's extended state */
>   void xstate_init(struct cpuinfo_x86 *c)
>   {
> -    static bool __initdata use_xsave = true;
> +    static bool use_xsave = true;

Please attach at least a brief comment here, such that people
won't consider the __initdata missing.

Of course I'd actually prefer the annotation to stay here in
the gcc case. Iirc there was one other case where there was
such an issue; I don't recall whether there it too got dealt
with by removing an annotation. How about we introduce an
annotation that expands to nothing in the clang case, but
continues to provide the same functionality for gcc? That
would then also clarify the reason for it being in any
particular place (I guess there are going to be more) without
the need for further commentary.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Mon, Jul 01, 2019 at 11:39:16AM +0000, Jan Beulich wrote:
> On 01.07.2019 12:49, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/xstate.c
> > +++ b/xen/arch/x86/xstate.c
> > @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
> >   /* Collect the information of processor's extended state */
> >   void xstate_init(struct cpuinfo_x86 *c)
> >   {
> > -    static bool __initdata use_xsave = true;
> > +    static bool use_xsave = true;
> 
> Please attach at least a brief comment here, such that people
> won't consider the __initdata missing.

Sure.

> 
> Of course I'd actually prefer the annotation to stay here in
> the gcc case. Iirc there was one other case where there was
> such an issue; I don't recall whether there it too got dealt
> with by removing an annotation.

Yes, in that other case the annotation was just removed, it's 43fa95ae [0]

> How about we introduce an
> annotation that expands to nothing in the clang case, but
> continues to provide the same functionality for gcc? That
> would then also clarify the reason for it being in any
> particular place (I guess there are going to be more) without
> the need for further commentary.

IMO that's a little bit dangerous, from the LLVM bug report it seems
like LLVM behaviour is not a bug, and hence I wouldn't be surprised if
newer versions of gcc also exhibit the same issue.

Thanks, Roger.

[0] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=43fa95ae6a64132b8ebe3025bd187ab9df68677b

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 01.07.2019 16:01, Roger Pau Monné  wrote:
> On Mon, Jul 01, 2019 at 11:39:16AM +0000, Jan Beulich wrote:
>> On 01.07.2019 12:49, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/xstate.c
>>> +++ b/xen/arch/x86/xstate.c
>>> @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>>>    /* Collect the information of processor's extended state */
>>>    void xstate_init(struct cpuinfo_x86 *c)
>>>    {
>>> -    static bool __initdata use_xsave = true;
>>> +    static bool use_xsave = true;
>>
>> Please attach at least a brief comment here, such that people
>> won't consider the __initdata missing.
> 
> Sure.
> 
>>
>> Of course I'd actually prefer the annotation to stay here in
>> the gcc case. Iirc there was one other case where there was
>> such an issue; I don't recall whether there it too got dealt
>> with by removing an annotation.
> 
> Yes, in that other case the annotation was just removed, it's 43fa95ae [0]
> 
>> How about we introduce an
>> annotation that expands to nothing in the clang case, but
>> continues to provide the same functionality for gcc? That
>> would then also clarify the reason for it being in any
>> particular place (I guess there are going to be more) without
>> the need for further commentary.
> 
> IMO that's a little bit dangerous, from the LLVM bug report it seems
> like LLVM behaviour is not a bug, and hence I wouldn't be surprised if
> newer versions of gcc also exhibit the same issue.

Okay, then let's go the route you suggested, just with a comment
added to prevent re-addition of the annotation. I wonder whether
we ought to do an audit of the code to find possible further
offenders. It doesn't look very desirable to me to find the
instances one by one by someone observing crashes.

Hmm, having looked at the older commit I'm again starting to wonder
whether dropping the annotations is the right course of action. Did
we consider adding volatile to the variable back then? That should
make the compiler not pull ahead the memory access(es), and the
downsides of this should be pretty limited for init-only items.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On Mon, Jul 01, 2019 at 02:20:52PM +0000, Jan Beulich wrote:
> On 01.07.2019 16:01, Roger Pau Monné  wrote:
> > On Mon, Jul 01, 2019 at 11:39:16AM +0000, Jan Beulich wrote:
> >> On 01.07.2019 12:49, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/xstate.c
> >>> +++ b/xen/arch/x86/xstate.c
> >>> @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
> >>>    /* Collect the information of processor's extended state */
> >>>    void xstate_init(struct cpuinfo_x86 *c)
> >>>    {
> >>> -    static bool __initdata use_xsave = true;
> >>> +    static bool use_xsave = true;
> >>
> >> Please attach at least a brief comment here, such that people
> >> won't consider the __initdata missing.
> > 
> > Sure.
> > 
> >>
> >> Of course I'd actually prefer the annotation to stay here in
> >> the gcc case. Iirc there was one other case where there was
> >> such an issue; I don't recall whether there it too got dealt
> >> with by removing an annotation.
> > 
> > Yes, in that other case the annotation was just removed, it's 43fa95ae [0]
> > 
> >> How about we introduce an
> >> annotation that expands to nothing in the clang case, but
> >> continues to provide the same functionality for gcc? That
> >> would then also clarify the reason for it being in any
> >> particular place (I guess there are going to be more) without
> >> the need for further commentary.
> > 
> > IMO that's a little bit dangerous, from the LLVM bug report it seems
> > like LLVM behaviour is not a bug, and hence I wouldn't be surprised if
> > newer versions of gcc also exhibit the same issue.
> 
> Okay, then let's go the route you suggested, just with a comment
> added to prevent re-addition of the annotation. I wonder whether
> we ought to do an audit of the code to find possible further
> offenders. It doesn't look very desirable to me to find the
> instances one by one by someone observing crashes.
> 
> Hmm, having looked at the older commit I'm again starting to wonder
> whether dropping the annotations is the right course of action. Did
> we consider adding volatile to the variable back then? That should
> make the compiler not pull ahead the memory access(es), and the
> downsides of this should be pretty limited for init-only items.

IIRC at some point I suggested using ACCESS_ONCE to read the init
opt_bootscrub variable. Adding the volatile keyword to the __initdata
macro is not going to work as-is, because we would also need to fixup
the existing declarations of the variables.

A more simple (and maybe easier to enforce rule) might be to forbid
the usage of init data/text in non-init functions, and that would
likely be easier to enforce automatically by some analysis tool.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 01.07.2019 17:16, Roger Pau Monné  wrote:
> On Mon, Jul 01, 2019 at 02:20:52PM +0000, Jan Beulich wrote:
>> On 01.07.2019 16:01, Roger Pau Monné  wrote:
>>> On Mon, Jul 01, 2019 at 11:39:16AM +0000, Jan Beulich wrote:
>>>> On 01.07.2019 12:49, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/xstate.c
>>>>> +++ b/xen/arch/x86/xstate.c
>>>>> @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>>>>>     /* Collect the information of processor's extended state */
>>>>>     void xstate_init(struct cpuinfo_x86 *c)
>>>>>     {
>>>>> -    static bool __initdata use_xsave = true;
>>>>> +    static bool use_xsave = true;
>>>>
>>>> Please attach at least a brief comment here, such that people
>>>> won't consider the __initdata missing.
>>>
>>> Sure.
>>>
>>>>
>>>> Of course I'd actually prefer the annotation to stay here in
>>>> the gcc case. Iirc there was one other case where there was
>>>> such an issue; I don't recall whether there it too got dealt
>>>> with by removing an annotation.
>>>
>>> Yes, in that other case the annotation was just removed, it's 43fa95ae [0]
>>>
>>>> How about we introduce an
>>>> annotation that expands to nothing in the clang case, but
>>>> continues to provide the same functionality for gcc? That
>>>> would then also clarify the reason for it being in any
>>>> particular place (I guess there are going to be more) without
>>>> the need for further commentary.
>>>
>>> IMO that's a little bit dangerous, from the LLVM bug report it seems
>>> like LLVM behaviour is not a bug, and hence I wouldn't be surprised if
>>> newer versions of gcc also exhibit the same issue.
>>
>> Okay, then let's go the route you suggested, just with a comment
>> added to prevent re-addition of the annotation. I wonder whether
>> we ought to do an audit of the code to find possible further
>> offenders. It doesn't look very desirable to me to find the
>> instances one by one by someone observing crashes.
>>
>> Hmm, having looked at the older commit I'm again starting to wonder
>> whether dropping the annotations is the right course of action. Did
>> we consider adding volatile to the variable back then? That should
>> make the compiler not pull ahead the memory access(es), and the
>> downsides of this should be pretty limited for init-only items.
> 
> IIRC at some point I suggested using ACCESS_ONCE to read the init
> opt_bootscrub variable. Adding the volatile keyword to the __initdata
> macro is not going to work as-is, because we would also need to fixup
> the existing declarations of the variables.

ACCESS_ONCE() is not good here because you need to then catch all
_uses_ of a variable. Dealing with it by changing the declaration/
definition makes it a one (or at most two) place change. I wouldn't
be worried about a header file also needing to change.

> A more simple (and maybe easier to enforce rule) might be to forbid
> the usage of init data/text in non-init functions, and that would
> likely be easier to enforce automatically by some analysis tool.

If you or anyone else is up for integrating such a tool with the
build system - fine. Right now we have none, and I don't want such
issues to be caught be solely dependent on the issue getting noticed
the latest during review.

That said, I'm not actually convinced unconditionally forbidding
such references (like Linux does, requiring further annotations to
override the ban) is the best model. There are clearly cases where
having such references is appropriate and useful. But I can accept
that Linux'es model is overall more safe than our current
effectively non-existent one.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel