[PATCH] mm: make opt_bootscrub non-init

[PATCH] mm: make opt_bootscrub non-init

[Roger Pau Monne]

LLVM code generation can attempt to load from a variable in the next
condition of an expression under certain circumstances, thus turning
the following condition:

if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )

Into:

0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
0xffff82d08022396e <+110>: setb   -0x29(%rbp)
0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>

Such code will trigger a page fault if system_state >=
SYS_STATE_active because opt_bootscrub will be unmapped.

Fix this by making opt_bootscrub 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

I haven't been able to find any other instances of such conditional
expression that uses system_state together with an init variable or
function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/common/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 08ee8cfbb9..56c0b24865 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -166,7 +166,7 @@ enum bootscrub_mode {
     BOOTSCRUB_ON,
     BOOTSCRUB_IDLE,
 };
-static enum bootscrub_mode __initdata opt_bootscrub = BOOTSCRUB_IDLE;
+static enum bootscrub_mode opt_bootscrub = BOOTSCRUB_IDLE;
 static int __init parse_bootscrub_param(const char *s)
 {
     /* Interpret 'bootscrub' alone in its positive boolean form */
-- 
2.19.1


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

Re: [PATCH] mm: make opt_bootscrub non-init

[Andrew Cooper]

On 23/11/2018 14:30, Roger Pau Monne wrote:
> LLVM code generation can attempt to load from a variable in the next
> condition of an expression under certain circumstances, thus turning
> the following condition:
>
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>
> Into:
>
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
>
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active because opt_bootscrub will be unmapped.
>
> Fix this by making opt_bootscrub 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
>
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/common/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 08ee8cfbb9..56c0b24865 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -166,7 +166,7 @@ enum bootscrub_mode {
>      BOOTSCRUB_ON,
>      BOOTSCRUB_IDLE,
>  };
> -static enum bootscrub_mode __initdata opt_bootscrub = BOOTSCRUB_IDLE;
> +static enum bootscrub_mode opt_bootscrub = BOOTSCRUB_IDLE;
>  static int __init parse_bootscrub_param(const char *s)
>  {
>      /* Interpret 'bootscrub' alone in its positive boolean form */


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

Re: [PATCH] mm: make opt_bootscrub non-init

[Julien Grall]

On 23/11/2018 14:30, Roger Pau Monne wrote:
> LLVM code generation can attempt to load from a variable in the next
> condition of an expression under certain circumstances, thus turning
> the following condition:
> 
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> 
> Into:
> 
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active because opt_bootscrub will be unmapped.
> 
> Fix this by making opt_bootscrub 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
> 
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>   xen/common/page_alloc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 08ee8cfbb9..56c0b24865 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -166,7 +166,7 @@ enum bootscrub_mode {
>       BOOTSCRUB_ON,
>       BOOTSCRUB_IDLE,
>   };
> -static enum bootscrub_mode __initdata opt_bootscrub = BOOTSCRUB_IDLE;
> +static enum bootscrub_mode opt_bootscrub = BOOTSCRUB_IDLE;
>   static int __init parse_bootscrub_param(const char *s)
>   {
>       /* Interpret 'bootscrub' alone in its positive boolean form */
> 

-- 
Julien Grall

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Sergey Dyasli]

On 23/11/2018 14:30, Roger Pau Monne wrote:
> LLVM code generation can attempt to load from a variable in the next
> condition of an expression under certain circumstances, thus turning
> the following condition:
> 
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> 
> Into:
> 
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active because opt_bootscrub will be unmapped.
> 
> Fix this by making opt_bootscrub 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
> 
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

I agree that not using __initdata variables in non-init functions is
the most sane approach.

--
Thanks,
Sergey

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Wei Liu]

On Fri, Nov 23, 2018 at 03:30:02PM +0100, Roger Pau Monne wrote:
> LLVM code generation can attempt to load from a variable in the next
> condition of an expression under certain circumstances, thus turning
> the following condition:
> 
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> 
> Into:
> 
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active because opt_bootscrub will be unmapped.
> 
> Fix this by making opt_bootscrub 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
> 
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Jan Beulich]

>>> On 23.11.18 at 15:30, <roger.pau@citrix.com> wrote:
> LLVM code generation can attempt to load from a variable in the next
> condition of an expression under certain circumstances, thus turning
> the following condition:
> 
> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> 
> Into:
> 
> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> 
> Such code will trigger a page fault if system_state >=
> SYS_STATE_active because opt_bootscrub will be unmapped.
> 
> Fix this by making opt_bootscrub 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 
> 
> I haven't been able to find any other instances of such conditional
> expression that uses system_state together with an init variable or
> function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I can accept this as a band-aid, so I'm not going to nack it, but
I don't view this as a feasible solution to the problem. That's in
particular because nothing is done at all to prevent future
similar issues. Even worse, ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -166,7 +166,7 @@ enum bootscrub_mode {
>      BOOTSCRUB_ON,
>      BOOTSCRUB_IDLE,
>  };
> -static enum bootscrub_mode __initdata opt_bootscrub = BOOTSCRUB_IDLE;
> +static enum bootscrub_mode opt_bootscrub = BOOTSCRUB_IDLE;

... no comment gets added here, which will make it rather likely
for someone to notice the missing __initdata and add it back. For
such a "trivial" change I wouldn't expect people to go do extra
archeology.

As an aside - __read_mostly should be added here instead.

Furthermore, while I trust your audit wrt system_state
accesses, these aren't the only potentially problematic ones.
For example in x86 specific code we pass around a boolean
indicating whether we're initializing the BSP or an AP. In other
places we compare the passed around struct cpuinfo_x86
pointer to the address of boot_cpu_data to tell apart the two
cases. The first example I can spot is guarding a function call
(to mcetelem_init()), so not a problem here, but I wouldn't
bet there are no __initdata variable accesses anywhere.

Jan


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

Re: [PATCH] mm: make opt_bootscrub non-init

[Roger Pau Monné]

On Mon, Nov 26, 2018 at 03:06:16AM -0700, Jan Beulich wrote:
> >>> On 23.11.18 at 15:30, <roger.pau@citrix.com> wrote:
> > LLVM code generation can attempt to load from a variable in the next
> > condition of an expression under certain circumstances, thus turning
> > the following condition:
> > 
> > if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> > 
> > Into:
> > 
> > 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 <system_state>
> > 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> > 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 <opt_bootscrub>
> > 
> > Such code will trigger a page fault if system_state >=
> > SYS_STATE_active because opt_bootscrub will be unmapped.
> > 
> > Fix this by making opt_bootscrub 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 
> > 
> > I haven't been able to find any other instances of such conditional
> > expression that uses system_state together with an init variable or
> > function.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I can accept this as a band-aid, so I'm not going to nack it, but
> I don't view this as a feasible solution to the problem. That's in
> particular because nothing is done at all to prevent future
> similar issues. Even worse, ...

I'm not sure what's the best way to prevent future issues. Should this
be mentioned in the coding style? That doesn't seems like the best
place, but I'm not sure where else could this be documented.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -166,7 +166,7 @@ enum bootscrub_mode {
> >      BOOTSCRUB_ON,
> >      BOOTSCRUB_IDLE,
> >  };
> > -static enum bootscrub_mode __initdata opt_bootscrub = BOOTSCRUB_IDLE;
> > +static enum bootscrub_mode opt_bootscrub = BOOTSCRUB_IDLE;
> 
> ... no comment gets added here, which will make it rather likely
> for someone to notice the missing __initdata and add it back. For
> such a "trivial" change I wouldn't expect people to go do extra
> archeology.
> 
> As an aside - __read_mostly should be added here instead.

I can add a comment, and yes, read_mostly should be used.

> Furthermore, while I trust your audit wrt system_state
> accesses, these aren't the only potentially problematic ones.
> For example in x86 specific code we pass around a boolean
> indicating whether we're initializing the BSP or an AP. In other
> places we compare the passed around struct cpuinfo_x86
> pointer to the address of boot_cpu_data to tell apart the two
> cases. The first example I can spot is guarding a function call
> (to mcetelem_init()), so not a problem here, but I wouldn't
> bet there are no __initdata variable accesses anywhere.

Hm, I guess we would need to use some kind of tool in order to detect
such accesses, Sparse maybe? But then we would likely have to match the
same rules as Linux, or modify Sparse to match our rules.

Thanks, Roger.

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Jan Beulich]

>>> On 26.11.18 at 13:04, <roger.pau@citrix.com> wrote:
> On Mon, Nov 26, 2018 at 03:06:16AM -0700, Jan Beulich wrote:
>> >>> On 23.11.18 at 15:30, <roger.pau@citrix.com> wrote:
>> > LLVM code generation can attempt to load from a variable in the next
>> > condition of an expression under certain circumstances, thus turning
>> > the following condition:
>> > 
>> > if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>> > 
>> > Into:
>> > 
>> > 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 
> <system_state>
>> > 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
>> > 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 
> <opt_bootscrub>
>> > 
>> > Such code will trigger a page fault if system_state >=
>> > SYS_STATE_active because opt_bootscrub will be unmapped.
>> > 
>> > Fix this by making opt_bootscrub 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 
>> > 
>> > I haven't been able to find any other instances of such conditional
>> > expression that uses system_state together with an init variable or
>> > function.
>> > 
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> I can accept this as a band-aid, so I'm not going to nack it, but
>> I don't view this as a feasible solution to the problem. That's in
>> particular because nothing is done at all to prevent future
>> similar issues. Even worse, ...
> 
> I'm not sure what's the best way to prevent future issues. Should this
> be mentioned in the coding style? That doesn't seems like the best
> place, but I'm not sure where else could this be documented.

There was some vaguely similar discussion a little while ago, and
there iirc we had also agreed that the point there (which I don't
recall) is not a style thing. Same here: We're talking about a
correctness issue, not a stylistic one. Hence indeed a separate
document would be needed, but none of the existing ones looks
to be a good fit.

Furthermore I doubt writing this down would help, because for
such apparently simple things no-one goes hunt for related
documentation. I think the only future proof course of action
would be to port Linux'es section mismatch handling and stop
allowing problematic cross references. That approach has
downsides though, which is why I'm not going to advocate it.

Jan


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

Re: [PATCH] mm: make opt_bootscrub non-init

[Julien Grall]

Hi Jan,

On 26/11/2018 12:18, Jan Beulich wrote:
>>>> On 26.11.18 at 13:04, <roger.pau@citrix.com> wrote:
>> On Mon, Nov 26, 2018 at 03:06:16AM -0700, Jan Beulich wrote:
>>>>>> On 23.11.18 at 15:30, <roger.pau@citrix.com> wrote:
>>>> LLVM code generation can attempt to load from a variable in the next
>>>> condition of an expression under certain circumstances, thus turning
>>>> the following condition:
>>>>
>>>> if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
>>>>
>>>> Into:
>>>>
>>>> 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0
>> <system_state>
>>>> 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
>>>> 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404
>> <opt_bootscrub>
>>>>
>>>> Such code will trigger a page fault if system_state >=
>>>> SYS_STATE_active because opt_bootscrub will be unmapped.
>>>>
>>>> Fix this by making opt_bootscrub 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
>>>>
>>>> I haven't been able to find any other instances of such conditional
>>>> expression that uses system_state together with an init variable or
>>>> function.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> I can accept this as a band-aid, so I'm not going to nack it, but
>>> I don't view this as a feasible solution to the problem. That's in
>>> particular because nothing is done at all to prevent future
>>> similar issues. Even worse, ...
>>
>> I'm not sure what's the best way to prevent future issues. Should this
>> be mentioned in the coding style? That doesn't seems like the best
>> place, but I'm not sure where else could this be documented.
> 
> There was some vaguely similar discussion a little while ago, and
> there iirc we had also agreed that the point there (which I don't
> recall) is not a style thing. Same here: We're talking about a
> correctness issue, not a stylistic one. Hence indeed a separate
> document would be needed, but none of the existing ones looks
> to be a good fit.
> 
> Furthermore I doubt writing this down would help, because for
> such apparently simple things no-one goes hunt for related
> documentation. I think the only future proof course of action
> would be to port Linux'es section mismatch handling and stop
> allowing problematic cross references. That approach has
> downsides though, which is why I'm not going to advocate it.

May I ask what are the downsides? Do you expect a lot of false positive?

Cheers,

-- 
Julien Grall

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Jan Beulich]

>>> On 26.11.18 at 13:25, <julien.grall@arm.com> wrote:
> May I ask what are the downsides? Do you expect a lot of false positive?

Having to split code paths, to introduce redundancy, or to move
code/data out of .init.* that could in fact live there are all
possible issues. Plus the __ref{,data} annotation that Linux has
also invite for abuse (at which point we'd be back to where we
currently are, just at perhaps a smaller scope).

Jan



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

Re: [PATCH] mm: make opt_bootscrub non-init

[Roger Pau Monné]

On Mon, Nov 26, 2018 at 05:18:20AM -0700, Jan Beulich wrote:
> >>> On 26.11.18 at 13:04, <roger.pau@citrix.com> wrote:
> > On Mon, Nov 26, 2018 at 03:06:16AM -0700, Jan Beulich wrote:
> >> >>> On 23.11.18 at 15:30, <roger.pau@citrix.com> wrote:
> >> > LLVM code generation can attempt to load from a variable in the next
> >> > condition of an expression under certain circumstances, thus turning
> >> > the following condition:
> >> > 
> >> > if ( system_state < SYS_STATE_active && opt_bootscrub == BOOTSCRUB_IDLE )
> >> > 
> >> > Into:
> >> > 
> >> > 0xffff82d080223967 <+103>: cmpl   $0x3,0x37b032(%rip) # 0xffff82d08059e9a0 
> > <system_state>
> >> > 0xffff82d08022396e <+110>: setb   -0x29(%rbp)
> >> > 0xffff82d080223972 <+114>: cmpl   $0x2,0x228a8b(%rip) # 0xffff82d08044c404 
> > <opt_bootscrub>
> >> > 
> >> > Such code will trigger a page fault if system_state >=
> >> > SYS_STATE_active because opt_bootscrub will be unmapped.
> >> > 
> >> > Fix this by making opt_bootscrub 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 
> >> > 
> >> > I haven't been able to find any other instances of such conditional
> >> > expression that uses system_state together with an init variable or
> >> > function.
> >> > 
> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> 
> >> I can accept this as a band-aid, so I'm not going to nack it, but
> >> I don't view this as a feasible solution to the problem. That's in
> >> particular because nothing is done at all to prevent future
> >> similar issues. Even worse, ...
> > 
> > I'm not sure what's the best way to prevent future issues. Should this
> > be mentioned in the coding style? That doesn't seems like the best
> > place, but I'm not sure where else could this be documented.
> 
> There was some vaguely similar discussion a little while ago, and
> there iirc we had also agreed that the point there (which I don't
> recall) is not a style thing. Same here: We're talking about a
> correctness issue, not a stylistic one. Hence indeed a separate
> document would be needed, but none of the existing ones looks
> to be a good fit.
> 
> Furthermore I doubt writing this down would help, because for
> such apparently simple things no-one goes hunt for related
> documentation. I think the only future proof course of action
> would be to port Linux'es section mismatch handling and stop
> allowing problematic cross references. That approach has
> downsides though, which is why I'm not going to advocate it.

Is Sparse the only option in this regard?

I think Andrew had played with Sparse on Xen before?

Albeit Linux and Xen share some similarities, I'm afraid that using
Sparse would mean either modifying Sparse itself, or modifying Xen to
match Linux. Are there any other options?

Thanks, Roger.

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

Re: [PATCH] mm: make opt_bootscrub non-init

[Jan Beulich]

>>> On 26.11.18 at 13:49, <roger.pau@citrix.com> wrote:
> On Mon, Nov 26, 2018 at 05:18:20AM -0700, Jan Beulich wrote:
>> Furthermore I doubt writing this down would help, because for
>> such apparently simple things no-one goes hunt for related
>> documentation. I think the only future proof course of action
>> would be to port Linux'es section mismatch handling and stop
>> allowing problematic cross references. That approach has
>> downsides though, which is why I'm not going to advocate it.
> 
> Is Sparse the only option in this regard?

I'm specifically not talking about sparse, but about Linux'es
logic in scripts/mod/modpost.c.

Jan



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