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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] mm: make opt_bootscrub non-init
Date: Mon, 26 Nov 2018 13:49:40 +0100	[thread overview]
Message-ID: <20181126124940.vjh7tm5ledpplkhq@mac> (raw)
In-Reply-To: <5BFBE48C02000078001FFF19@prv1-mh.provo.novell.com>

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