[Karmic] SRU-fix: Fix nx_enable reporting

As described in the patch, we got a regression in proposed which was
caused by a SRU update. Though fixing the message fir 64bit it caused
32bit non-PAE suddenly to believe it had nx protection. Which then
caused sleep problems (for the computer, not the programmer :)).
I guess after being involved in testing, I should get two other ACKs
for it and then respin, proposed. Thanks.

-Stefan

Hi,

On Tue, 2009-11-10 at 13:43 +0100, Stefan Bader wrote:
> As described in the patch, we got a regression in proposed which was
> caused by a SRU update. Though fixing the message fir 64bit it caused
> 32bit non-PAE suddenly to believe it had nx protection. Which then
> caused sleep problems (for the computer, not the programmer :)).
> I guess after being involved in testing, I should get two other ACKs
> for it and then respin, proposed. Thanks.

This is quite subtle, since _PAGE_NX is non-zero for CONFIG_X86_64 ||
CONFIG_X86_PAE configurations, hence zero for 32 bit systems without
PAE.  The effect of setting nx_enabled to zero for 32 bit systems
without PAE now means that header->pmode_efer_low is now set correctly
in acpi_save_state_mem() (sleep.c) for these systems, hence fixing the
suspend/resume issue.

So, functionally, yep, it looks OK.

Acked-by: Colin King <colin.king@canonical.com>

> 
> -Stefan
> 
>

On Tue, Nov 10, 2009 at 01:43:27PM +0100, Stefan Bader wrote:
> As described in the patch, we got a regression in proposed which was
> caused by a SRU update. Though fixing the message fir 64bit it caused
> 32bit non-PAE suddenly to believe it had nx protection. Which then
> caused sleep problems (for the computer, not the programmer :)).
> I guess after being involved in testing, I should get two other ACKs
> for it and then respin, proposed. Thanks.
> 
> -Stefan

> From cf0516662862b64f42ae167d50894d1152d27743 Mon Sep 17 00:00:00 2001
> From: Kees Cook <kees.cook@canonical.com>
> Date: Mon, 9 Nov 2009 13:58:54 -0800
> Subject: [PATCH] UBUNTU: SAUCE: Fix nx_enable reporting
> 
> BugLink: https://bugs.launchpad.net/bugs/454285
> Submitted upstream.
> 
> This fixes a regression caused by
> 
> commit 8bf095f8b62c5fdfe55a8c95be775ea62be7bc10
> Author: Kees Cook <kees@ubuntu.com>
> Date:   Sun Oct 18 09:16:44 2009 -0700
>     UBUNTU: SAUCE: [x86] fix report of cs-limit nx-emulation
> 
> The _PAGE_NX is 0 when compiled for 32bit without PAE, so the previous
> attempt to fix the misleading message on 64bit caused 32bit non-PAE to
> break (discovered by having problems on suspend/resume).
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> Tested-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/mm/init.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 817fa01..6654c44 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -77,7 +77,7 @@ static void __init set_nx(void)
>  #else
>  static inline void set_nx(void)
>  {
> -	nx_enabled = ( (__supported_pte_mask & _PAGE_NX) == _PAGE_NX );
> +	nx_enabled = _PAGE_NX && ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX);
>  }
>  #endif

The issue was that _PAGE_NX went to 0 when on i386 non-pae.  Such that
the expression became true inadvertantly:

  __support_pte_mask & 0 == 0

The change looks appropriate to fix that.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

On 09 Nov 10, Colin King wrote:
> Hi,
> 
> On Tue, 2009-11-10 at 13:43 +0100, Stefan Bader wrote:
> > As described in the patch, we got a regression in proposed which was
> > caused by a SRU update. Though fixing the message fir 64bit it caused
> > 32bit non-PAE suddenly to believe it had nx protection. Which then
> > caused sleep problems (for the computer, not the programmer :)).
> > I guess after being involved in testing, I should get two other ACKs
> > for it and then respin, proposed. Thanks.
> 
> This is quite subtle, since _PAGE_NX is non-zero for CONFIG_X86_64 ||
> CONFIG_X86_PAE configurations, hence zero for 32 bit systems without
> PAE.  The effect of setting nx_enabled to zero for 32 bit systems
> without PAE now means that header->pmode_efer_low is now set correctly
> in acpi_save_state_mem() (sleep.c) for these systems, hence fixing the
> suspend/resume issue.
> 
> So, functionally, yep, it looks OK.
> 
> Acked-by: Colin King <colin.king@canonical.com>

I understood the subtlety after reading Colin's explanation. I believe his
explanation should be part of the patch since it better explains the problem.


Conditionally-Acked-by: Amit Kucheria <amit.kucheria@canonical.com>

Amit Kucheria wrote:
> On 09 Nov 10, Colin King wrote:
>> Hi,
>>
>> On Tue, 2009-11-10 at 13:43 +0100, Stefan Bader wrote:
>>> As described in the patch, we got a regression in proposed which was
>>> caused by a SRU update. Though fixing the message fir 64bit it caused
>>> 32bit non-PAE suddenly to believe it had nx protection. Which then
>>> caused sleep problems (for the computer, not the programmer :)).
>>> I guess after being involved in testing, I should get two other ACKs
>>> for it and then respin, proposed. Thanks.
>> This is quite subtle, since _PAGE_NX is non-zero for CONFIG_X86_64 ||
>> CONFIG_X86_PAE configurations, hence zero for 32 bit systems without
>> PAE.  The effect of setting nx_enabled to zero for 32 bit systems
>> without PAE now means that header->pmode_efer_low is now set correctly
>> in acpi_save_state_mem() (sleep.c) for these systems, hence fixing the
>> suspend/resume issue.
>>
>> So, functionally, yep, it looks OK.
>>
>> Acked-by: Colin King <colin.king@canonical.com>
> 
> I understood the subtlety after reading Colin's explanation. I believe his
> explanation should be part of the patch since it better explains the problem.
> 
> 
> Conditionally-Acked-by: Amit Kucheria <amit.kucheria@canonical.com>
> 

Yeah, what Amit said.