Re: [Xen-devel] kasan_map_early_shadow() on Xen

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[David Vrabel]

On 03/03/15 09:40, Luis R. Rodriguez wrote:
> Andrey,
> 
> I believe that on Xen we should disable kasan, would like confirmation

Why?  This is the first of heard of this.

> from someone on xen-devel though. Here's the thing though -- if true
> -- I'd like to do it *properly*, where *properly* means addressing a
> bit of architecture. A simple Kconfig slap seems rather reactive. I'd
> like to address a way to properly ensure we don't run into this and
> other similar issues in the future. The CR4 shadow issue was another
> recent example issue, also introduced via v4.0 [0]. We can't keep
> doing this reactively.
> 
> Let's go down the rabbit hole for a bit. HAVE_ARCH_KASAN will be
> selected on x86 when:
> 
> if X86_64 && SPARSEMEM_VMEMMAP
> 
> Now Xen should not have SPARSEMEM_VMEMMAP but PVOPs' goal is to enable

Why?  Again, this is the first I've heard of this as well.  FWIW, all
the Xen configs we use have SPARSEMEM_VMEMMAP enabled.

David

> distributions to be able to have a single binary kernels and let the
> rest be figured out, so we can't just disable SPARSEMEM_VMEMMAP for
> Xen alone, we want to build Xen.. or part of Xen and perhaps keep
> SPARSEMEM_VMEMMAP, and only later figure things out.
> 
> How do we do this cleanly and avoid future reactive measures? If the
> answer is not upon us, I'd like to at least highlight the issue so
> that in case we do come up with something its no surprise PVOPs is
> falling short for that single binary pipe dream right now.
> 
> [0] https://lkml.org/lkml/2015/2/23/328
> 
>  Luis
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Jan Beulich]

>>> On 03.03.15 at 10:40, <mcgrof@suse.com> wrote:
> Let's go down the rabbit hole for a bit. HAVE_ARCH_KASAN will be
> selected on x86 when:
> 
> if X86_64 && SPARSEMEM_VMEMMAP
> 
> Now Xen should not have SPARSEMEM_VMEMMAP

Why would that be?

Jan

Re: kasan_map_early_shadow() on Xen

[Andrey Ryabinin]

On 03/03/2015 12:40 PM, Luis R. Rodriguez wrote:
> Andrey,
> 
> I believe that on Xen we should disable kasan, would like confirmation

I guess Xen guests won't work with kasan because Xen guests doesn't setup shadow
(kasan_map_early_shadow() is not called in xen guests).

Disabling kasan for Xen in Kconfig is undesirable because that will disable kasan
for allmodconfig and allyesconfig builds, but I don't see other option for now.


> from someone on xen-devel though. Here's the thing though -- if true
> -- I'd like to do it *properly*, where *properly* means addressing a
> bit of architecture. A simple Kconfig slap seems rather reactive. I'd
> like to address a way to properly ensure we don't run into this and
> other similar issues in the future. The CR4 shadow issue was another
> recent example issue, also introduced via v4.0 [0]. We can't keep
> doing this reactively.
> 
> Let's go down the rabbit hole for a bit. HAVE_ARCH_KASAN will be
> selected on x86 when:
> 
> if X86_64 && SPARSEMEM_VMEMMAP
> 
> Now Xen should not have SPARSEMEM_VMEMMAP but PVOPs' goal is to enable
> distributions to be able to have a single binary kernels and let the
> rest be figured out, so we can't just disable SPARSEMEM_VMEMMAP for
> Xen alone, we want to build Xen.. or part of Xen and perhaps keep
> SPARSEMEM_VMEMMAP, and only later figure things out.
> 
> How do we do this cleanly and avoid future reactive measures? If the
> answer is not upon us, I'd like to at least highlight the issue so
> that in case we do come up with something its no surprise PVOPs is
> falling short for that single binary pipe dream right now.
> 
> [0] https://lkml.org/lkml/2015/2/23/328
> 
>  Luis
> 

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Konrad Rzeszutek Wilk]

On Tue, Mar 03, 2015 at 04:15:06PM +0300, Andrey Ryabinin wrote:
> On 03/03/2015 12:40 PM, Luis R. Rodriguez wrote:
> > Andrey,
> > 
> > I believe that on Xen we should disable kasan, would like confirmation
> 
> I guess Xen guests won't work with kasan because Xen guests doesn't setup shadow
> (kasan_map_early_shadow() is not called in xen guests).
> 
> Disabling kasan for Xen in Kconfig is undesirable because that will disable kasan
> for allmodconfig and allyesconfig builds, but I don't see other option for now.

Was there an bug reported for this? It would be good to CC the maintainers
of Xen on that sort of thing.

Thanks!
.. snip..
> > [0] https://lkml.org/lkml/2015/2/23/328

If you look at the other x86 pulls - you will see that the authors
also missed enabling it on 32-bit kernels!

> > 
> >  Luis
> > 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Andrey Ryabinin]

On 03/03/2015 05:16 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 03, 2015 at 04:15:06PM +0300, Andrey Ryabinin wrote:
>> On 03/03/2015 12:40 PM, Luis R. Rodriguez wrote:
>>> Andrey,
>>>
>>> I believe that on Xen we should disable kasan, would like confirmation
>>
>> I guess Xen guests won't work with kasan because Xen guests doesn't setup shadow
>> (kasan_map_early_shadow() is not called in xen guests).
>>
>> Disabling kasan for Xen in Kconfig is undesirable because that will disable kasan
>> for allmodconfig and allyesconfig builds, but I don't see other option for now.
> 
> Was there an bug reported for this? It would be good to CC the maintainers
> of Xen on that sort of thing.
> 

There was no report for this. I just looked at Xen code because of this Luis's mail
and I don't see how it could work with kasan.
Fixing this might be not trivial. We need to setup shadow memory before any kasan instrumented
function will run.
This is like with -fstack-protector (you need to setup gdt before calling any stack protected function).

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Konrad Rzeszutek Wilk]

On Tue, Mar 03, 2015 at 06:38:20PM +0300, Andrey Ryabinin wrote:
> On 03/03/2015 05:16 PM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Mar 03, 2015 at 04:15:06PM +0300, Andrey Ryabinin wrote:
> >> On 03/03/2015 12:40 PM, Luis R. Rodriguez wrote:
> >>> Andrey,
> >>>
> >>> I believe that on Xen we should disable kasan, would like confirmation
> >>
> >> I guess Xen guests won't work with kasan because Xen guests doesn't setup shadow
> >> (kasan_map_early_shadow() is not called in xen guests).
> >>
> >> Disabling kasan for Xen in Kconfig is undesirable because that will disable kasan
> >> for allmodconfig and allyesconfig builds, but I don't see other option for now.
> > 
> > Was there an bug reported for this? It would be good to CC the maintainers
> > of Xen on that sort of thing.
> > 
> 
> There was no report for this. I just looked at Xen code because of this Luis's mail
> and I don't see how it could work with kasan.

Ahh.
> Fixing this might be not trivial. We need to setup shadow memory before any kasan instrumented
> function will run.
> This is like with -fstack-protector (you need to setup gdt before calling any stack protected function).

If it is like that - then just using what had to be implemented
for the stack protection as a template ought to pave most of the
work?

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Andrey Ryabinin]

On 03/03/2015 07:02 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 03, 2015 at 06:38:20PM +0300, Andrey Ryabinin wrote:
>> On 03/03/2015 05:16 PM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Mar 03, 2015 at 04:15:06PM +0300, Andrey Ryabinin wrote:
>>>> On 03/03/2015 12:40 PM, Luis R. Rodriguez wrote:
>>>>> Andrey,
>>>>>
>>>>> I believe that on Xen we should disable kasan, would like confirmation
>>>>
>>>> I guess Xen guests won't work with kasan because Xen guests doesn't setup shadow
>>>> (kasan_map_early_shadow() is not called in xen guests).
>>>>
>>>> Disabling kasan for Xen in Kconfig is undesirable because that will disable kasan
>>>> for allmodconfig and allyesconfig builds, but I don't see other option for now.
>>>
>>> Was there an bug reported for this? It would be good to CC the maintainers
>>> of Xen on that sort of thing.
>>>
>>
>> There was no report for this. I just looked at Xen code because of this Luis's mail
>> and I don't see how it could work with kasan.
> 
> Ahh.
>> Fixing this might be not trivial. We need to setup shadow memory before any kasan instrumented
>> function will run.
>> This is like with -fstack-protector (you need to setup gdt before calling any stack protected function).
> 
> If it is like that - then just using what had to be implemented
> for the stack protection as a template ought to pave most of the
> work?
> 

Probably. I think I could make this work.
However, I won't be able to work on this until the end of the next week.

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Luis R. Rodriguez]

On Wed, Mar 4, 2015 at 6:36 AM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> On 03/03/2015 07:02 PM, Konrad Rzeszutek Wilk wrote:
>> If it is like that - then just using what had to be implemented
>> for the stack protection as a template ought to pave most of the
>> work?
>
> Probably. I think I could make this work.
> However, I won't be able to work on this until the end of the next week.

While a solution is likely possible here I'd like for us to notice how
two features have gone now under our nose for Xen on v4.0 which have
issues. Proactive maintainer reviews would detect these issues but we
can't bet on these, and testing is just as reactive and slow. I'm
hinting we keep our eyes out for an architectural solution that would
avoid these issues. Don't ask me what that is just yet...

 Luis

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Konrad Rzeszutek Wilk]

On Wed, Mar 04, 2015 at 05:47:03PM -0800, Luis R. Rodriguez wrote:
> On Wed, Mar 4, 2015 at 6:36 AM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> > On 03/03/2015 07:02 PM, Konrad Rzeszutek Wilk wrote:
> >> If it is like that - then just using what had to be implemented
> >> for the stack protection as a template ought to pave most of the
> >> work?
> >
> > Probably. I think I could make this work.
> > However, I won't be able to work on this until the end of the next week.
> 
> While a solution is likely possible here I'd like for us to notice how
> two features have gone now under our nose for Xen on v4.0 which have
> issues. Proactive maintainer reviews would detect these issues but we
> can't bet on these, and testing is just as reactive and slow. I'm

Hmm, I see you saying 'Proactive maintainer review would detect' these
but that assumes that the maintainer would even be copied on these patches?

None of the Xen maintainers were copied on this.

And while we all strive to be pro-active I have to remind you maintainers
are usually swamped.

> hinting we keep our eyes out for an architectural solution that would
> avoid these issues. Don't ask me what that is just yet...

Keep in mind that a lot of these issues get resolved during merge window
thanks to the resiliancy of Boris monitoring these tests with an sharp eye!

After all that is what merge windows are - things break during them.

> 
>  Luis

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Luis R. Rodriguez]

On Fri, Mar 06, 2015 at 11:02:50AM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 04, 2015 at 05:47:03PM -0800, Luis R. Rodriguez wrote:
> > On Wed, Mar 4, 2015 at 6:36 AM, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> > > On 03/03/2015 07:02 PM, Konrad Rzeszutek Wilk wrote:
> > >> If it is like that - then just using what had to be implemented
> > >> for the stack protection as a template ought to pave most of the
> > >> work?
> > >
> > > Probably. I think I could make this work.
> > > However, I won't be able to work on this until the end of the next week.
> > 
> > While a solution is likely possible here I'd like for us to notice how
> > two features have gone now under our nose for Xen on v4.0 which have
> > issues. Proactive maintainer reviews would detect these issues but we
> > can't bet on these, and testing is just as reactive and slow. I'm
> 
> Hmm, I see you saying 'Proactive maintainer review would detect' these
> but that assumes that the maintainer would even be copied on these patches?
> 
> None of the Xen maintainers were copied on this.

That is a damn shame. More on this below.

> And while we all strive to be pro-active I have to remind you maintainers
> are usually swamped.

Right, I expected this, its why I in no way am saying its the roles
which would cause this.

> > hinting we keep our eyes out for an architectural solution that would
> > avoid these issues. Don't ask me what that is just yet...
> 
> Keep in mind that a lot of these issues get resolved during merge window
> thanks to the resiliancy of Boris monitoring these tests with an sharp eye!

If this is about testing then its reactive. Its still great work but reactive
is never better than a proactive engineering solution.

> After all that is what merge windows are - things break during them.

Sure. What was highlighting, and now with a bit different emphasis, is the
fragile nature of using different entry points for both PV Xen and bare metal
x86_64 init, as well as other things involved with Xen which require careful
surgery and analysis. I was alluding to the possibility of seeing if we can
somehow draw up a solution to make it rather hard for these sorts of issues
to creep up again and for Xen to reactively correct them, with what you point
out that even the Xen maintainers were not copied on these patches it makes
this a bit more of an important issue otherwise we're going to be in the same
situation for v4.1, v4.2 and so on.

If I had a technical solution to this problem I'd propose it but I don't,
I'm just flagging it right now and hope we can come up with one.

 Luis

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Luis R. Rodriguez]

On Fri, Mar 6, 2015 at 8:47 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> If I had a technical solution to this problem I'd propose it but I don't,
> I'm just flagging it right now and hope we can come up with one.

Shortly I'll be posting part of a proactive solution I believe we
could consider for these sorts of issues.

 Luis

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Luis R. Rodriguez]

On Tue, Mar 3, 2015 at 2:06 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> On 03/03/15 09:40, Luis R. Rodriguez wrote:
>> Andrey,
>>
>> I believe that on Xen we should disable kasan, would like confirmation
>
> Why?  This is the first of heard of this.

Andrey chimed in here confirming this.

>> from someone on xen-devel though. Here's the thing though -- if true
>> -- I'd like to do it *properly*, where *properly* means addressing a
>> bit of architecture. A simple Kconfig slap seems rather reactive. I'd
>> like to address a way to properly ensure we don't run into this and
>> other similar issues in the future. The CR4 shadow issue was another
>> recent example issue, also introduced via v4.0 [0]. We can't keep
>> doing this reactively.
>>
>> Let's go down the rabbit hole for a bit. HAVE_ARCH_KASAN will be
>> selected on x86 when:
>>
>> if X86_64 && SPARSEMEM_VMEMMAP
>>
>> Now Xen should not have SPARSEMEM_VMEMMAP but PVOPs' goal is to enable
>
> Why?  Again, this is the first I've heard of this as well.  FWIW, all
> the Xen configs we use have SPARSEMEM_VMEMMAP enabled.

Interesting... we have config ARCH_SPARSEMEM_ENABLE depend on !XEN at
SUSE. Figured this was a generic issue. The SUSE kernels are based on
3.12 though, but anyway with it enabled I do get compile failures
because of redefinition of MAX_PHYSMEM_BITS which we provide on Xen
set to 43 for some reason (can't find that justification), so it
doesn't use the default 46 that would be used otherwise. But another
reason seems to be the lack of forward porting yet PAT support for PV
domains -- commit 47591df50 upstream which requires us to still have
the union on the pte_t, and I suppose we need ca15f20f as well...

If there is nothing else I suppose this just requires fixing up at
SUSE's end for SPARSEMEM_VMEMMAP...

--- ./arch/x86/include/asm/pgtable_64_types.h   2015-03-02
13:35:49.885257763 -0800
+++ ./arch/x86/include/mach-xen/asm/pgtable_64_types.h  2015-03-02
13:36:25.554259348 -0800
@@ -15,7 +15,7 @@
 typedef unsigned long  pgdval_t;
 typedef unsigned long  pgprotval_t;

-typedef struct { pteval_t pte; } pte_t;
+typedef union { pteval_t pte; unsigned int pte_low; } pte_t;

 #endif /* !__ASSEMBLY__ */

@@ -54,6 +54,7 @@
 #define PGDIR_MASK     (~(PGDIR_SIZE - 1))

 /* See Documentation/x86/x86_64/mm.txt for a description of the memory map. */
+#define MAX_PHYSMEM_BITS 43
 #define MAXMEM          _AC(__AC(1, UL) << MAX_PHYSMEM_BITS, UL)
 #define VMALLOC_START    _AC(0xffffc90000000000, UL)
 #define VMALLOC_END      _AC(0xffffe8ffffffffff, UL)

 Luis

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Juergen Gross]

On 03/03/2015 08:20 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 3, 2015 at 2:06 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>> On 03/03/15 09:40, Luis R. Rodriguez wrote:
>>> Andrey,
>>>
>>> I believe that on Xen we should disable kasan, would like confirmation
>>
>> Why?  This is the first of heard of this.
>
> Andrey chimed in here confirming this.
>
>>> from someone on xen-devel though. Here's the thing though -- if true
>>> -- I'd like to do it *properly*, where *properly* means addressing a
>>> bit of architecture. A simple Kconfig slap seems rather reactive. I'd
>>> like to address a way to properly ensure we don't run into this and
>>> other similar issues in the future. The CR4 shadow issue was another
>>> recent example issue, also introduced via v4.0 [0]. We can't keep
>>> doing this reactively.
>>>
>>> Let's go down the rabbit hole for a bit. HAVE_ARCH_KASAN will be
>>> selected on x86 when:
>>>
>>> if X86_64 && SPARSEMEM_VMEMMAP
>>>
>>> Now Xen should not have SPARSEMEM_VMEMMAP but PVOPs' goal is to enable
>>
>> Why?  Again, this is the first I've heard of this as well.  FWIW, all
>> the Xen configs we use have SPARSEMEM_VMEMMAP enabled.
>
> Interesting... we have config ARCH_SPARSEMEM_ENABLE depend on !XEN at
> SUSE. Figured this was a generic issue. The SUSE kernels are based on
> 3.12 though, but anyway with it enabled I do get compile failures
> because of redefinition of MAX_PHYSMEM_BITS which we provide on Xen
> set to 43 for some reason (can't find that justification), so it
> doesn't use the default 46 that would be used otherwise. But another
> reason seems to be the lack of forward porting yet PAT support for PV
> domains -- commit 47591df50 upstream which requires us to still have
> the union on the pte_t, and I suppose we need ca15f20f as well...
>
> If there is nothing else I suppose this just requires fixing up at
> SUSE's end for SPARSEMEM_VMEMMAP...

The SUSE kernel has several patches renaming/altering Xen-related config
options. Don't mix that up with upstream/pvops.

Juergen

Re: [Xen-devel] kasan_map_early_shadow() on Xen

[Jan Beulich]

>>> On 04.03.15 at 05:53, <JGross@suse.com> wrote:
> On 03/03/2015 08:20 PM, Luis R. Rodriguez wrote:
>> On Tue, Mar 3, 2015 at 2:06 AM, David Vrabel <david.vrabel@citrix.com> wrote:
>>> On 03/03/15 09:40, Luis R. Rodriguez wrote:
>>>> if X86_64 && SPARSEMEM_VMEMMAP
>>>>
>>>> Now Xen should not have SPARSEMEM_VMEMMAP but PVOPs' goal is to enable
>>>
>>> Why?  Again, this is the first I've heard of this as well.  FWIW, all
>>> the Xen configs we use have SPARSEMEM_VMEMMAP enabled.
>>
>> Interesting... we have config ARCH_SPARSEMEM_ENABLE depend on !XEN at
>> SUSE. Figured this was a generic issue. The SUSE kernels are based on
>> 3.12 though, but anyway with it enabled I do get compile failures
>> because of redefinition of MAX_PHYSMEM_BITS which we provide on Xen
>> set to 43 for some reason (can't find that justification), so it
>> doesn't use the default 46 that would be used otherwise. But another
>> reason seems to be the lack of forward porting yet PAT support for PV
>> domains -- commit 47591df50 upstream which requires us to still have
>> the union on the pte_t, and I suppose we need ca15f20f as well...
>>
>> If there is nothing else I suppose this just requires fixing up at
>> SUSE's end for SPARSEMEM_VMEMMAP...
> 
> The SUSE kernel has several patches renaming/altering Xen-related config
> options. Don't mix that up with upstream/pvops.

Exactly - some of what our kernels do in this regard could be useful
upstream, while others (like this one) won't be. Prior to making claims
against upstream kernels you should tell between these cases.

Jan