x86/mm/pageattr: Code without effect?

x86/mm/pageattr: Code without effect?

[Stefan Bader]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

When looking through some mm code I stumbled over one part in
arch/x86/mm/pageattr.c that looks somewhat bogus to me. Cannot
say what exactly the effects are, but maybe you do (or you could
explain to me why I am wrong :)).

commit a8aed3e0752b4beb2e37cbed6df69faae88268da
Author: Andrea Arcangeli <aarcange@redhat.com>
Date:   Fri Feb 22 15:11:51 2013 -0800

    x86/mm/pageattr: Prevent PSE and GLOABL leftovers to confuse
    pmd/pte_present and pmd_huge

added the following to try_preserve_large_page:

 	/*
+	 * Set the PSE and GLOBAL flags only if the PRESENT flag is
+	 * set otherwise pmd_present/pmd_huge will return true even on
+	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
+	 * for the ancient hardware that doesn't support it.
+	 */
+	if (pgprot_val(new_prot) & _PAGE_PRESENT)
+		pgprot_val(new_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
+	else
+		pgprot_val(new_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
+
+	new_prot = canon_pgprot(new_prot);
+
+	/*

but (extending what follows after the changes)

	 * old_pte points to the large page base address. So we need
	 * to add the offset of the virtual address:
	 */
	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
	cpa->pfn = pfn;

	new_prot = static_protections(req_prot, address, pfn);

So new_prot gets completely replaced by req_prot and all changes done to
new_prot before look to be lost (the PSE and GLOBAL bit settings as well
as the canon_pgprot call.

Maybe the hunk is useless anyway, or the breakage is subtle, or I miss something...

Thanks,
Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: x86/mm/pageattr: Code without effect?

[Borislav Petkov]

On Fri, Apr 05, 2013 at 11:01:02AM +0200, Stefan Bader wrote:
> When looking through some mm code I stumbled over one part in
> arch/x86/mm/pageattr.c that looks somewhat bogus to me. Cannot
> say what exactly the effects are, but maybe you do (or you could
> explain to me why I am wrong :)).
> 
> commit a8aed3e0752b4beb2e37cbed6df69faae88268da
> Author: Andrea Arcangeli <aarcange@redhat.com>
> Date:   Fri Feb 22 15:11:51 2013 -0800
> 
>     x86/mm/pageattr: Prevent PSE and GLOABL leftovers to confuse
>     pmd/pte_present and pmd_huge
> 
> added the following to try_preserve_large_page:
> 
>  	/*
> +	 * Set the PSE and GLOBAL flags only if the PRESENT flag is
> +	 * set otherwise pmd_present/pmd_huge will return true even on
> +	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
> +	 * for the ancient hardware that doesn't support it.
> +	 */
> +	if (pgprot_val(new_prot) & _PAGE_PRESENT)
> +		pgprot_val(new_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
> +	else
> +		pgprot_val(new_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
> +
> +	new_prot = canon_pgprot(new_prot);
> +
> +	/*
> 
> but (extending what follows after the changes)
> 
> 	 * old_pte points to the large page base address. So we need
> 	 * to add the offset of the virtual address:
> 	 */
> 	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
> 	cpa->pfn = pfn;
> 
> 	new_prot = static_protections(req_prot, address, pfn);
> 
> So new_prot gets completely replaced by req_prot and all changes done to
> new_prot before look to be lost (the PSE and GLOBAL bit settings as well
> as the canon_pgprot call.
> 
> Maybe the hunk is useless anyway, or the breakage is subtle, or I miss something...

Yeah, I had to unwillingly stare at this crazy code recently too and
I can share your confusion. And from trying to grok what's going
on, I *think* what we actually meant to do is sanitize our required
protections first, i.e.

	new_prot = static_protections(req_prot, address, pfn);

and *then* do the _PAGE_PRESENT massaging. It does at least make sense
that way. And this is what we already do in __change_page_attr() for a
4K pte.

Andrea?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: x86/mm/pageattr: Code without effect?

[Andrea Arcangeli]

Hi everyone,

On Fri, Apr 05, 2013 at 04:21:04PM +0200, Borislav Petkov wrote:
> On Fri, Apr 05, 2013 at 11:01:02AM +0200, Stefan Bader wrote:
> > When looking through some mm code I stumbled over one part in
> > arch/x86/mm/pageattr.c that looks somewhat bogus to me. Cannot
> > say what exactly the effects are, but maybe you do (or you could
> > explain to me why I am wrong :)).
> > 
> > commit a8aed3e0752b4beb2e37cbed6df69faae88268da
> > Author: Andrea Arcangeli <aarcange@redhat.com>
> > Date:   Fri Feb 22 15:11:51 2013 -0800
> > 
> >     x86/mm/pageattr: Prevent PSE and GLOABL leftovers to confuse
> >     pmd/pte_present and pmd_huge
> > 
> > added the following to try_preserve_large_page:
> > 
> >  	/*
> > +	 * Set the PSE and GLOBAL flags only if the PRESENT flag is
> > +	 * set otherwise pmd_present/pmd_huge will return true even on
> > +	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
> > +	 * for the ancient hardware that doesn't support it.
> > +	 */
> > +	if (pgprot_val(new_prot) & _PAGE_PRESENT)
> > +		pgprot_val(new_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
> > +	else
> > +		pgprot_val(new_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
> > +
> > +	new_prot = canon_pgprot(new_prot);
> > +
> > +	/*
> > 
> > but (extending what follows after the changes)
> > 
> > 	 * old_pte points to the large page base address. So we need
> > 	 * to add the offset of the virtual address:
> > 	 */
> > 	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
> > 	cpa->pfn = pfn;
> > 
> > 	new_prot = static_protections(req_prot, address, pfn);
> > 
> > So new_prot gets completely replaced by req_prot and all changes done to
> > new_prot before look to be lost (the PSE and GLOBAL bit settings as well
> > as the canon_pgprot call.
> > 
> > Maybe the hunk is useless anyway, or the breakage is subtle, or I miss something...
> 
> Yeah, I had to unwillingly stare at this crazy code recently too and
> I can share your confusion. And from trying to grok what's going
> on, I *think* what we actually meant to do is sanitize our required
> protections first, i.e.
> 
> 	new_prot = static_protections(req_prot, address, pfn);
> 
> and *then* do the _PAGE_PRESENT massaging. It does at least make sense
> that way. And this is what we already do in __change_page_attr() for a
> 4K pte.
> 
> Andrea?

You're right, so this location clearly didn't trigger the problem so I
didn't notice the noop here. I only exercised the fix in the other
locations of the file that had the same problem.

It was a noop, so it really couldn't hurt but the below change should
activate the fix there too. On the same lines, there was a superfluous
initialization of new_prot too which I cleaned up.

==
>From 75598be1156ced0c210271e8958a5c5714a2626a Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Fri, 5 Apr 2013 19:43:20 +0200
Subject: [PATCH] mm: pageattr: convert noop to functional fix

commit a8aed3e0752b4beb2e37cbed6df69faae88268da introduced some valid
fix but one location that didn't trigger the bug that lead to finding
those (small) problems, wasn't updated using the right variable.

The wrong variable was also initialized for no good reason, that may
have been the source of the confusion. Remove the noop initialization
accordingly.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 arch/x86/mm/pageattr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 091934e..7896f71 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -467,7 +467,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	 * We are safe now. Check whether the new pgprot is the same:
 	 */
 	old_pte = *kpte;
-	old_prot = new_prot = req_prot = pte_pgprot(old_pte);
+	old_prot = req_prot = pte_pgprot(old_pte);
 
 	pgprot_val(req_prot) &= ~pgprot_val(cpa->mask_clr);
 	pgprot_val(req_prot) |= pgprot_val(cpa->mask_set);
@@ -478,12 +478,12 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
 	 * for the ancient hardware that doesn't support it.
 	 */
-	if (pgprot_val(new_prot) & _PAGE_PRESENT)
-		pgprot_val(new_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
+	if (pgprot_val(req_prot) & _PAGE_PRESENT)
+		pgprot_val(req_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
 	else
-		pgprot_val(new_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
+		pgprot_val(req_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
 
-	new_prot = canon_pgprot(new_prot);
+	req_prot = canon_pgprot(req_prot);
 
 	/*
 	 * old_pte points to the large page base address. So we need

Re: x86/mm/pageattr: Code without effect?

[Borislav Petkov]

On Sat, Apr 06, 2013 at 04:58:04PM +0200, Andrea Arcangeli wrote:
> You're right, so this location clearly didn't trigger the problem so I
> didn't notice the noop here. I only exercised the fix in the other
> locations of the file that had the same problem.
> 
> It was a noop, so it really couldn't hurt but the below change should
> activate the fix there too. On the same lines, there was a superfluous
> initialization of new_prot too which I cleaned up.
> 
> ==
> From 75598be1156ced0c210271e8958a5c5714a2626a Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Fri, 5 Apr 2013 19:43:20 +0200
> Subject: [PATCH] mm: pageattr: convert noop to functional fix
> 
> commit a8aed3e0752b4beb2e37cbed6df69faae88268da introduced some valid
> fix but one location that didn't trigger the bug that lead to finding
> those (small) problems, wasn't updated using the right variable.
> 
> The wrong variable was also initialized for no good reason, that may
> have been the source of the confusion. Remove the noop initialization
> accordingly.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>

Yeah, looks good to me. I've folded it into my pile of changes touching
this and there are no visible issues. I mean, if there were, we
should've noticed it exploding elsewhere by now. Also, you might want to
credit Stefan in the commit message for spotting this.

Acked-by: Borislav Petkov <bp@suse.de>

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: x86/mm/pageattr: Code without effect?

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> > have been the source of the confusion. Remove the noop initialization
> > accordingly.
> > 
> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> 
> Yeah, looks good to me. I've folded it into my pile of changes touching this and 
> there are no visible issues. [...]

Logistics question: is this fix coming upstream-wards via your pile of changes 
anytime soon?

Thanks,

	Ingo

Re: x86/mm/pageattr: Code without effect?

[Borislav Petkov]

On Mon, Apr 08, 2013 at 01:53:44PM +0200, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@alien8.de> wrote:
> 
> > > have been the source of the confusion. Remove the noop initialization
> > > accordingly.
> > > 
> > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > 
> > Yeah, looks good to me. I've folded it into my pile of changes touching this and 
> > there are no visible issues. [...]
> 
> Logistics question: is this fix coming upstream-wards via your pile of changes 
> anytime soon?

Actually I was thinking Andrea would send it since it is his fix. And
besides, my pile is still stinking. :-)

AFAICT, the patch fixes a noop so the current code works anyway - IOW,
it is basically a code correctness fix which doesn't have any other
effect. What I mean by that is, no need to go in now for 3.9 and stable.

Long story, short: best it would be, IMO, if Andrea would send it to
you soonish but you apply it for 3.10 so that it sees a whole cycle of
testing just in case - it is CPA code after all.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: x86/mm/pageattr: Code without effect?

[Stefan Bader]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On 08.04.2013 13:59, Borislav Petkov wrote:
> On Mon, Apr 08, 2013 at 01:53:44PM +0200, Ingo Molnar wrote:
>>
>> * Borislav Petkov <bp@alien8.de> wrote:
>>
>>>> have been the source of the confusion. Remove the noop initialization
>>>> accordingly.
>>>>
>>>> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
>>>
>>> Yeah, looks good to me. I've folded it into my pile of changes touching this and 
>>> there are no visible issues. [...]
>>
>> Logistics question: is this fix coming upstream-wards via your pile of changes 
>> anytime soon?
> 
> Actually I was thinking Andrea would send it since it is his fix. And
> besides, my pile is still stinking. :-)
> 
> AFAICT, the patch fixes a noop so the current code works anyway - IOW,
> it is basically a code correctness fix which doesn't have any other
> effect. What I mean by that is, no need to go in now for 3.9 and stable.
> 
> Long story, short: best it would be, IMO, if Andrea would send it to
> you soonish but you apply it for 3.10 so that it sees a whole cycle of
> testing just in case - it is CPA code after all.

To me it would read as someone or something may use change page attribute to set
PSE or GLOBAL (or any or the masked off bits from canon_pgprot) in an unexpected
way for a huge page as long as it is not split.
As Boris says, it has not happened as far as it is known. Probably even if it
happens it is one of those "on the right day of week when the phase of the moon
is right" errors...
To enforce the PSE bit here sounds reasonably right. And also apply
canon_pgprot, too. GLOBAL I don't know for sure.

By the way there is a usage of new_prot a bit down of try_preserve_large_page
which probably should be changed into req_prot, too. That was enforcing the
canon_pgprot before the change. So that may be considered a regression to before.

-Stefan
> 
> Thanks.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: x86/mm/pageattr: Code without effect?

[Borislav Petkov]

On Mon, Apr 08, 2013 at 02:28:47PM +0200, Stefan Bader wrote:
> To enforce the PSE bit here sounds reasonably right. And also apply
> canon_pgprot, too. GLOBAL I don't know for sure.

Well sure, you don't want to flush those from the TLB if it is kernel
memory since it is mapped in every process AFAICT.

> By the way there is a usage of new_prot a bit down of
> try_preserve_large_page which probably should be changed into
> req_prot, too. That was enforcing the canon_pgprot before the change.
> So that may be considered a regression to before.

Which one?

Actually, after Andrea's patch it all makes sense - we initialize
new_prot from req_prot *after* all protections checks. new_prot are,
IMHO, the final protection bits which we are actually going to change.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: x86/mm/pageattr: Code without effect?

[Stefan Bader]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On 08.04.2013 14:51, Borislav Petkov wrote:
> On Mon, Apr 08, 2013 at 02:28:47PM +0200, Stefan Bader wrote:
>> To enforce the PSE bit here sounds reasonably right. And also apply
>> canon_pgprot, too. GLOBAL I don't know for sure.
> 
> Well sure, you don't want to flush those from the TLB if it is kernel
> memory since it is mapped in every process AFAICT.
> 
>> By the way there is a usage of new_prot a bit down of
>> try_preserve_large_page which probably should be changed into
>> req_prot, too. That was enforcing the canon_pgprot before the change.
>> So that may be considered a regression to before.
> 
> Which one?

         * that we limited the number of possible pages already to
         * the number of pages in the large page.
         */
        if (address == (address & pmask) && cpa->numpages == (psize >>
PAGE_SHIFT)) {
                /*
                 * The address is aligned and the number of pages
                 * covers the full page.
                 */
                new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
                                                     ^

This one. The first patch changed

-               new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
+               new_pte = pfn_pte(pte_pfn(old_pte), new_prot);

The fixup patch drops new_prot completely from being initialized and only works
on req_prot. Probably it would be best to also drop the definition of new_prot.
I think it then completely unused.

-Stefan

> 
> Actually, after Andrea's patch it all makes sense - we initialize
> new_prot from req_prot *after* all protections checks. new_prot are,
> IMHO, the final protection bits which we are actually going to change.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: x86/mm/pageattr: Code without effect?

[Borislav Petkov]

On Mon, Apr 08, 2013 at 03:10:00PM +0200, Stefan Bader wrote:
>          * that we limited the number of possible pages already to
>          * the number of pages in the large page.
>          */
>         if (address == (address & pmask) && cpa->numpages == (psize >>
> PAGE_SHIFT)) {
>                 /*
>                  * The address is aligned and the number of pages
>                  * covers the full page.
>                  */
>                 new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
>                                                      ^
> 
> This one. The first patch changed
> 
> -               new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
> +               new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
> 
> The fixup patch drops new_prot completely from being initialized and only works
> on req_prot. Probably it would be best to also drop the definition of new_prot.
> I think it then completely unused.

Actually, we do need and initialize new_prot at line 495:

	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
	cpa->pfn = pfn;

	new_prot = static_protections(req_prot, address, pfn);		<---

and we need it for the subsequent loop where we go over the 512 PTEs to
decide whether to split or not.

So it is needed after all, AFAICT.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: x86/mm/pageattr: Code without effect?

[Stefan Bader]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On 08.04.2013 16:15, Borislav Petkov wrote:
> On Mon, Apr 08, 2013 at 03:10:00PM +0200, Stefan Bader wrote:
>>          * that we limited the number of possible pages already to
>>          * the number of pages in the large page.
>>          */
>>         if (address == (address & pmask) && cpa->numpages == (psize >>
>> PAGE_SHIFT)) {
>>                 /*
>>                  * The address is aligned and the number of pages
>>                  * covers the full page.
>>                  */
>>                 new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
>>                                                      ^
>>
>> This one. The first patch changed
>>
>> -               new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
>> +               new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
>>
>> The fixup patch drops new_prot completely from being initialized and only works
>> on req_prot. Probably it would be best to also drop the definition of new_prot.
>> I think it then completely unused.
> 
> Actually, we do need and initialize new_prot at line 495:
> 
> 	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
> 	cpa->pfn = pfn;
> 
> 	new_prot = static_protections(req_prot, address, pfn);		<---

You are right. Seems I missed that and a couble of other places. I can see them
now... Hm, Monday morning or just morning issue... So, yes, its new_prot is
initialized and is still needed, otherwise  the loop over the whole range would
be subtly different.
Sorry for the noise.

-Stefan

> 
> and we need it for the subsequent loop where we go over the 512 PTEs to
> decide whether to split or not.
> 
> So it is needed after all, AFAICT.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: x86/mm/pageattr: Code without effect?

[Andy Whitcroft]

On Sat, Apr 06, 2013 at 04:58:04PM +0200, Andrea Arcangeli wrote:

> You're right, so this location clearly didn't trigger the problem so I
> didn't notice the noop here. I only exercised the fix in the other
> locations of the file that had the same problem.
> 
> It was a noop, so it really couldn't hurt but the below change should
> activate the fix there too. On the same lines, there was a superfluous
> initialization of new_prot too which I cleaned up.

Although the new code is essentially noop, the other part of the change
in try_preserve_large_page() moves the canon_pgprot() up above the
static_protections() incantation which replaces its value, thus we lose
the effect of that on the protection bits.  I suspect this only affects
older CPUs (?) but I do think there is a negative semantic change here:

+       new_prot = canon_pgprot(new_prot);
[...]
        new_prot = static_protections(req_prot, address, pfn);
[...]
-               new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
+               new_pte = pfn_pte(pte_pfn(old_pte), new_prot);

-apw

Re: x86/mm/pageattr: Code without effect?

[Andrea Arcangeli]

On Mon, Apr 08, 2013 at 03:53:31PM +0100, Andy Whitcroft wrote:
> On Sat, Apr 06, 2013 at 04:58:04PM +0200, Andrea Arcangeli wrote:
> 
> > You're right, so this location clearly didn't trigger the problem so I
> > didn't notice the noop here. I only exercised the fix in the other
> > locations of the file that had the same problem.
> > 
> > It was a noop, so it really couldn't hurt but the below change should
> > activate the fix there too. On the same lines, there was a superfluous
> > initialization of new_prot too which I cleaned up.
> 
> Although the new code is essentially noop, the other part of the change
> in try_preserve_large_page() moves the canon_pgprot() up above the
> static_protections() incantation which replaces its value, thus we lose
> the effect of that on the protection bits.  I suspect this only affects
> older CPUs (?) but I do think there is a negative semantic change here:
> 
> +       new_prot = canon_pgprot(new_prot);
> [...]
>         new_prot = static_protections(req_prot, address, pfn);
> [...]
> -               new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
> +               new_pte = pfn_pte(pte_pfn(old_pte), new_prot);

Agreed, canon_pgprot is only for old cpus so it went unnoticed. The
patch I posted yesterday will fix that too.

static_protections only clear bits. canon_pgprot only clear bits
too. So the order won't matter. And unless we want to enforce
canon_pgprot to always run after static_protections for whatever
reason it should be fine now. If you want to enforce canon_pgprot to
always run last let me know.

Thanks,
Andrea