Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 2018-09-04 at 18:52 +0200, Thomas Gleixner wrote:
> On Tue, 4 Sep 2018, Thomas Gleixner wrote:
> > On Tue, 4 Sep 2018, Yang, Bin wrote:
> > > On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > > > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned
> > > > > long address,
> > > > >
> > > > > psize = page_level_size(level);
> > > > > pmask = page_level_mask(level);
> > > > > + addr = address & pmask;
> > > > >
> > > > > /*
> > > > >* Calculate the number of pages, which fit into this large
> > > > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned
> > > > > long address,
> > > > > cpa->numpages = numpages;
> > > > >
> > > > > /*
> > > > > + * The old pgprot should not have any protection bit. Otherwise,
> > > > > + * the existing mapping is wrong already.
> > > > > + */
> > > > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize,
> > > > > old_pfn));
> > > >
> > > > The check itself is fine, but it just emits a warning and goes on as if
> > > > nothing happened.
> > > >
> > > > We really want to think about a proper way to fix that up without
> > > > overhead
> > > > for the sane case.
> > >
> > > could we change it as below? I think it should be safe to split large
> > > page if current mapping is wrong already.
> > >
> > > if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> > > WARN_ON_ONCE(1);
> > > goto out_unlock;
> > > }
> >
> > Sure, but what enforces the static protections on the pages which are not
> > in the modified range of the current CPA call? Nothing.
>
> I looked deeper into that. For the PMD split it's rather trivial to do, but
> a PUD split would require a horrible pile of changes as we'd have to remove
> the protections from the new PMD first, go all the way back and rescan the
> new PMDs whether they need to be split up further. But that needs a lot of
> refactoring and I'm not sure if it's worth the trouble right now.
>
> As we haven't cared about that since CPA got introduced, I think we just do
> the consistency check and warn. That's better what we have now and when it
> ever triggers revisit it.
Is below check enough?
if ((pgprot_val(old_prot) & _PAGE_PRESENT) &&
needs_static_protections(old_prot, addr, psize, old_pfn)) {
WARN_ON_ONCE(1);
}
>
> Thanks,
>
> tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 2018-09-04 at 18:52 +0200, Thomas Gleixner wrote:
> On Tue, 4 Sep 2018, Thomas Gleixner wrote:
> > On Tue, 4 Sep 2018, Yang, Bin wrote:
> > > On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > > > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned
> > > > > long address,
> > > > >
> > > > > psize = page_level_size(level);
> > > > > pmask = page_level_mask(level);
> > > > > + addr = address & pmask;
> > > > >
> > > > > /*
> > > > >* Calculate the number of pages, which fit into this large
> > > > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned
> > > > > long address,
> > > > > cpa->numpages = numpages;
> > > > >
> > > > > /*
> > > > > + * The old pgprot should not have any protection bit. Otherwise,
> > > > > + * the existing mapping is wrong already.
> > > > > + */
> > > > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize,
> > > > > old_pfn));
> > > >
> > > > The check itself is fine, but it just emits a warning and goes on as if
> > > > nothing happened.
> > > >
> > > > We really want to think about a proper way to fix that up without
> > > > overhead
> > > > for the sane case.
> > >
> > > could we change it as below? I think it should be safe to split large
> > > page if current mapping is wrong already.
> > >
> > > if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> > > WARN_ON_ONCE(1);
> > > goto out_unlock;
> > > }
> >
> > Sure, but what enforces the static protections on the pages which are not
> > in the modified range of the current CPA call? Nothing.
>
> I looked deeper into that. For the PMD split it's rather trivial to do, but
> a PUD split would require a horrible pile of changes as we'd have to remove
> the protections from the new PMD first, go all the way back and rescan the
> new PMDs whether they need to be split up further. But that needs a lot of
> refactoring and I'm not sure if it's worth the trouble right now.
>
> As we haven't cared about that since CPA got introduced, I think we just do
> the consistency check and warn. That's better what we have now and when it
> ever triggers revisit it.
Is below check enough?
if ((pgprot_val(old_prot) & _PAGE_PRESENT) &&
needs_static_protections(old_prot, addr, psize, old_pfn)) {
WARN_ON_ONCE(1);
}
>
> Thanks,
>
> tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 4 Sep 2018, Thomas Gleixner wrote:
> On Tue, 4 Sep 2018, Yang, Bin wrote:
> > On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > > address,
> > > >
> > > > psize = page_level_size(level);
> > > > pmask = page_level_mask(level);
> > > > + addr = address & pmask;
> > > >
> > > > /*
> > > > * Calculate the number of pages, which fit into this large
> > > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > > address,
> > > > cpa->numpages = numpages;
> > > >
> > > > /*
> > > > +* The old pgprot should not have any protection bit. Otherwise,
> > > > +* the existing mapping is wrong already.
> > > > +*/
> > > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize,
> > > > old_pfn));
> > >
> > > The check itself is fine, but it just emits a warning and goes on as if
> > > nothing happened.
> > >
> > > We really want to think about a proper way to fix that up without overhead
> > > for the sane case.
> >
> > could we change it as below? I think it should be safe to split large
> > page if current mapping is wrong already.
> >
> > if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> > WARN_ON_ONCE(1);
> > goto out_unlock;
> > }
>
> Sure, but what enforces the static protections on the pages which are not
> in the modified range of the current CPA call? Nothing.
I looked deeper into that. For the PMD split it's rather trivial to do, but
a PUD split would require a horrible pile of changes as we'd have to remove
the protections from the new PMD first, go all the way back and rescan the
new PMDs whether they need to be split up further. But that needs a lot of
refactoring and I'm not sure if it's worth the trouble right now.
As we haven't cared about that since CPA got introduced, I think we just do
the consistency check and warn. That's better what we have now and when it
ever triggers revisit it.
Thanks,
tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 4 Sep 2018, Thomas Gleixner wrote:
> On Tue, 4 Sep 2018, Yang, Bin wrote:
> > On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > > address,
> > > >
> > > > psize = page_level_size(level);
> > > > pmask = page_level_mask(level);
> > > > + addr = address & pmask;
> > > >
> > > > /*
> > > > * Calculate the number of pages, which fit into this large
> > > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > > address,
> > > > cpa->numpages = numpages;
> > > >
> > > > /*
> > > > +* The old pgprot should not have any protection bit. Otherwise,
> > > > +* the existing mapping is wrong already.
> > > > +*/
> > > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize,
> > > > old_pfn));
> > >
> > > The check itself is fine, but it just emits a warning and goes on as if
> > > nothing happened.
> > >
> > > We really want to think about a proper way to fix that up without overhead
> > > for the sane case.
> >
> > could we change it as below? I think it should be safe to split large
> > page if current mapping is wrong already.
> >
> > if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> > WARN_ON_ONCE(1);
> > goto out_unlock;
> > }
>
> Sure, but what enforces the static protections on the pages which are not
> in the modified range of the current CPA call? Nothing.
I looked deeper into that. For the PMD split it's rather trivial to do, but
a PUD split would require a horrible pile of changes as we'd have to remove
the protections from the new PMD first, go all the way back and rescan the
new PMDs whether they need to be split up further. But that needs a lot of
refactoring and I'm not sure if it's worth the trouble right now.
As we haven't cared about that since CPA got introduced, I think we just do
the consistency check and warn. That's better what we have now and when it
ever triggers revisit it.
Thanks,
tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 4 Sep 2018, Yang, Bin wrote:
> On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > address,
> > >
> > > psize = page_level_size(level);
> > > pmask = page_level_mask(level);
> > > + addr = address & pmask;
> > >
> > > /*
> > >* Calculate the number of pages, which fit into this large
> > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > address,
> > > cpa->numpages = numpages;
> > >
> > > /*
> > > + * The old pgprot should not have any protection bit. Otherwise,
> > > + * the existing mapping is wrong already.
> > > + */
> > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn));
> >
> > The check itself is fine, but it just emits a warning and goes on as if
> > nothing happened.
> >
> > We really want to think about a proper way to fix that up without overhead
> > for the sane case.
>
> could we change it as below? I think it should be safe to split large
> page if current mapping is wrong already.
>
> if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> WARN_ON_ONCE(1);
> goto out_unlock;
> }
Sure, but what enforces the static protections on the pages which are not
in the modified range of the current CPA call? Nothing.
And the above is also wrong because you unconditionally check even if the
existing pgprot has the PRESENT bit cleared. Guess what happens.
Thanks,
tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 4 Sep 2018, Yang, Bin wrote:
> On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> > On Tue, 21 Aug 2018, Bin Yang wrote:
> > > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > address,
> > >
> > > psize = page_level_size(level);
> > > pmask = page_level_mask(level);
> > > + addr = address & pmask;
> > >
> > > /*
> > >* Calculate the number of pages, which fit into this large
> > > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > > address,
> > > cpa->numpages = numpages;
> > >
> > > /*
> > > + * The old pgprot should not have any protection bit. Otherwise,
> > > + * the existing mapping is wrong already.
> > > + */
> > > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn));
> >
> > The check itself is fine, but it just emits a warning and goes on as if
> > nothing happened.
> >
> > We really want to think about a proper way to fix that up without overhead
> > for the sane case.
>
> could we change it as below? I think it should be safe to split large
> page if current mapping is wrong already.
>
> if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
> WARN_ON_ONCE(1);
> goto out_unlock;
> }
Sure, but what enforces the static protections on the pages which are not
in the modified range of the current CPA call? Nothing.
And the above is also wrong because you unconditionally check even if the
existing pgprot has the PRESENT bit cleared. Guess what happens.
Thanks,
tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> On Tue, 21 Aug 2018, Bin Yang wrote:
> > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > address,
> >
> > psize = page_level_size(level);
> > pmask = page_level_mask(level);
> > + addr = address & pmask;
> >
> > /*
> > * Calculate the number of pages, which fit into this large
> > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > address,
> > cpa->numpages = numpages;
> >
> > /*
> > +* The old pgprot should not have any protection bit. Otherwise,
> > +* the existing mapping is wrong already.
> > +*/
> > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn));
>
> The check itself is fine, but it just emits a warning and goes on as if
> nothing happened.
>
> We really want to think about a proper way to fix that up without overhead
> for the sane case.
could we change it as below? I think it should be safe to split large
page if current mapping is wrong already.
if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
WARN_ON_ONCE(1);
goto out_unlock;
}
>
> Thanks,
>
> tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 2018-09-04 at 00:27 +0200, Thomas Gleixner wrote:
> On Tue, 21 Aug 2018, Bin Yang wrote:
> > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > address,
> >
> > psize = page_level_size(level);
> > pmask = page_level_mask(level);
> > + addr = address & pmask;
> >
> > /*
> > * Calculate the number of pages, which fit into this large
> > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long
> > address,
> > cpa->numpages = numpages;
> >
> > /*
> > +* The old pgprot should not have any protection bit. Otherwise,
> > +* the existing mapping is wrong already.
> > +*/
> > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn));
>
> The check itself is fine, but it just emits a warning and goes on as if
> nothing happened.
>
> We really want to think about a proper way to fix that up without overhead
> for the sane case.
could we change it as below? I think it should be safe to split large
page if current mapping is wrong already.
if (needs_static_protections(old_prot, addr, psize, old_pfn)) {
WARN_ON_ONCE(1);
goto out_unlock;
}
>
> Thanks,
>
> tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 21 Aug 2018, Bin Yang wrote: > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long > address, > > psize = page_level_size(level); > pmask = page_level_mask(level); > + addr = address & pmask; > > /* >* Calculate the number of pages, which fit into this large > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long > address, > cpa->numpages = numpages; > > /* > + * The old pgprot should not have any protection bit. Otherwise, > + * the existing mapping is wrong already. > + */ > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn)); The check itself is fine, but it just emits a warning and goes on as if nothing happened. We really want to think about a proper way to fix that up without overhead for the sane case. Thanks, tglx
Re: [PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
On Tue, 21 Aug 2018, Bin Yang wrote: > @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long > address, > > psize = page_level_size(level); > pmask = page_level_mask(level); > + addr = address & pmask; > > /* >* Calculate the number of pages, which fit into this large > @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long > address, > cpa->numpages = numpages; > > /* > + * The old pgprot should not have any protection bit. Otherwise, > + * the existing mapping is wrong already. > + */ > + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn)); The check itself is fine, but it just emits a warning and goes on as if nothing happened. We really want to think about a proper way to fix that up without overhead for the sane case. Thanks, tglx
[PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
If there is a large page which contains an area which requires a different mapping that the one which the large page provides, then something went wrong _before_ this code is called. Here we can catch a case where the existing mapping is wrong already. Inspired-by: Thomas Gleixner Signed-off-by: Bin Yang --- arch/x86/mm/pageattr.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c index fd90c5b..91a250c 100644 --- a/arch/x86/mm/pageattr.c +++ b/arch/x86/mm/pageattr.c @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, psize = page_level_size(level); pmask = page_level_mask(level); + addr = address & pmask; /* * Calculate the number of pages, which fit into this large @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, cpa->numpages = numpages; /* +* The old pgprot should not have any protection bit. Otherwise, +* the existing mapping is wrong already. +*/ + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn)); + + /* * We are safe now. Check whether the new pgprot is the same: * Convert protection attributes to 4k-format, as cpa->mask* are set * up accordingly. @@ -690,7 +697,6 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, * would anyway result in a split after doing all the check work * for nothing. */ - addr = address & pmask; if (address != addr || cpa->numpages != numpages) goto out_unlock; -- 2.7.4
[PATCH v3 5/5] x86/mm: add WARN_ON_ONCE() for wrong large page mapping
If there is a large page which contains an area which requires a different mapping that the one which the large page provides, then something went wrong _before_ this code is called. Here we can catch a case where the existing mapping is wrong already. Inspired-by: Thomas Gleixner Signed-off-by: Bin Yang --- arch/x86/mm/pageattr.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c index fd90c5b..91a250c 100644 --- a/arch/x86/mm/pageattr.c +++ b/arch/x86/mm/pageattr.c @@ -625,6 +625,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, psize = page_level_size(level); pmask = page_level_mask(level); + addr = address & pmask; /* * Calculate the number of pages, which fit into this large @@ -636,6 +637,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, cpa->numpages = numpages; /* +* The old pgprot should not have any protection bit. Otherwise, +* the existing mapping is wrong already. +*/ + WARN_ON_ONCE(needs_static_protections(old_prot, addr, psize, old_pfn)); + + /* * We are safe now. Check whether the new pgprot is the same: * Convert protection attributes to 4k-format, as cpa->mask* are set * up accordingly. @@ -690,7 +697,6 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, * would anyway result in a split after doing all the check work * for nothing. */ - addr = address & pmask; if (address != addr || cpa->numpages != numpages) goto out_unlock; -- 2.7.4
