set_memory_rw on the kernel text

set_memory_rw on the kernel text

[Michal Hocko]

Hi Subject,
I was debugging an issue that the kernel text didn't get remapped RW
after set_memory_rw and generated a #PF even though set_memory_rw
returned with success (0). I am completely unfamiliar with the code
but it become clear from the code inspection that static_protections()
will drop _PAGE_RW from the protection flags with CONFIG_DEBUG_RODATA
for the large mappings. try_preserve_large_page will then interpret this
as no change is needed and return with 0 all the way up to the caller.

I can see the point that set_memory_rw doesn't allow remapping after
certain moment (kernel_set_to_readonly is non-zero) but the current
semantic with returning success even though the operation was ignored is
strange.

Shouldn't the function return -EPERM instead? So that the caller doesn't
try to write to the address and #PF? Something like a completely
untested.

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 89af288ec674..c1fcb02f9662 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -540,6 +540,10 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	cpa->pfn = pfn;
 
 	new_prot = static_protections(req_prot, address, pfn);
+	if (pgprot_val(new_prot) ^ pgprot_val(req_prot)) {
+		do_split = -EPERM;
+		goto out_unlock;
+	}
 
 	/*
 	 * We need to check the full range, whether

I am also trying to understand why the semantic is different for 4k
pages. I can see that pmd prot change might influence different sections
in the same pmd range or something like that but why don't we simply
split the pmd then and make the 4k page RW?
-- 
Michal Hocko
SUSE Labs

Re: set_memory_rw on the kernel text

[Michal Hocko]

On Tue 11-08-15 16:18:45, Michal Hocko wrote:
> Hi Subject,

Dang, Ctrl+p in vim. Sorry about that...
-- 
Michal Hocko
SUSE Labs

Re: set_memory_rw on the kernel text

[Jiri Kosina]

On Tue, 11 Aug 2015, Michal Hocko wrote:

> [reposted with the correct address and name]
> 
> Hi Suresh,
> I was debugging an issue that the kernel text didn't get remapped RW
> after set_memory_rw and generated a #PF even though set_memory_rw
> returned with success (0). I am completely unfamiliar with the code
> but it become clear from the code inspection that static_protections()
> will drop _PAGE_RW from the protection flags with CONFIG_DEBUG_RODATA
> for the large mappings. try_preserve_large_page will then interpret this
> as no change is needed and return with 0 all the way up to the caller.
> 
> I can see the point that set_memory_rw doesn't allow remapping after
> certain moment (kernel_set_to_readonly is non-zero) but the current
> semantic with returning success even though the operation was ignored is
> strange.
> 
> Shouldn't the function return -EPERM instead? So that the caller doesn't
> try to write to the address and #PF? Something like a completely
> untested.

I think this would be a nice to have fix. Suresh ... ?

> 
> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> index 89af288ec674..c1fcb02f9662 100644
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -540,6 +540,10 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
>  	cpa->pfn = pfn;
>  
>  	new_prot = static_protections(req_prot, address, pfn);
> +	if (pgprot_val(new_prot) ^ pgprot_val(req_prot)) {
> +		do_split = -EPERM;
> +		goto out_unlock;
> +	}
>  
>  	/*
>  	 * We need to check the full range, whether
> 
> I am also trying to understand why the semantic is different for 4k
> pages. I can see that pmd prot change might influence different sections
> in the same pmd range or something like that but why don't we simply
> split the pmd then and make the 4k page RW?

-- 
Jiri Kosina
SUSE Labs

set_memory_rw on the kernel text

[Michal Hocko]

[reposted with the correct address and name]

Hi Suresh,
I was debugging an issue that the kernel text didn't get remapped RW
after set_memory_rw and generated a #PF even though set_memory_rw
returned with success (0). I am completely unfamiliar with the code
but it become clear from the code inspection that static_protections()
will drop _PAGE_RW from the protection flags with CONFIG_DEBUG_RODATA
for the large mappings. try_preserve_large_page will then interpret this
as no change is needed and return with 0 all the way up to the caller.

I can see the point that set_memory_rw doesn't allow remapping after
certain moment (kernel_set_to_readonly is non-zero) but the current
semantic with returning success even though the operation was ignored is
strange.

Shouldn't the function return -EPERM instead? So that the caller doesn't
try to write to the address and #PF? Something like a completely
untested.

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 89af288ec674..c1fcb02f9662 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -540,6 +540,10 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	cpa->pfn = pfn;
 
 	new_prot = static_protections(req_prot, address, pfn);
+	if (pgprot_val(new_prot) ^ pgprot_val(req_prot)) {
+		do_split = -EPERM;
+		goto out_unlock;
+	}
 
 	/*
 	 * We need to check the full range, whether

I am also trying to understand why the semantic is different for 4k
pages. I can see that pmd prot change might influence different sections
in the same pmd range or something like that but why don't we simply
split the pmd then and make the 4k page RW?
-- 
Michal Hocko
SUSE Labs