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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 26, 2018 at 03:06:16AM -0700, Jan Beulich wrote:
> >>> On 23.11.18 at 15:30, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel