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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.11.18 at 13:04, <roger.pau@xxxxxxxxxx> wrote:
> 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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel