Skip to contentSkip to conversation

Use correct page protection for put_dirty_page

PATCH In review Andi Kleen 3 participants 5 messages May 11, 2003

Subsystems (4)
VFS MM core s390 exec and binfmt
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Use correct page protection for put_dirty_page

put_page_dirty must use the page protection of the stack VMA, not hardcoded PAGE_COPY. They can be different e.g. when the stack is set non executable via VM_STACK_FLAGS.

I added an fallback path for now because I'm not sure if the stack VMA is always predowngrowed here, if the printk better it may be also needed to add an stack extension here.

-Andi

fs/exec.c renamed from fs/exec.c-o +9 -1
@@ -292,7 +292,9 @@
 	pgd_t * pgd;
 	pmd_t * pmd;
 	pte_t * pte;
+	pgprot_t prot;
 	struct pte_chain *pte_chain;
+	struct vm_area_struct *vma;
 
 	if (page_count(page) != 1)
 		printk(KERN_ERR "mem_map disagrees with %p at %08lx\n", page, address);

@@ -314,7 +316,13 @@
 	}
 	lru_cache_add_active(page);
 	flush_dcache_page(page);
-	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, PAGE_COPY))));
+	vma = find_vma(tsk->mm, address);
+	if (!vma) { 
+		printk("put_dirty_page: stack vma not extended yet %lx?\n",address); 
+		prot = PAGE_COPY;
+	} else
+		prot = vma->vm_page_prot;
+	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, prot))));
 	pte_chain = page_add_rmap(page, pte, pte_chain);
 	pte_unmap(pte);
 	tsk->mm->rss++;
#1 lore raw
Quoted 6 lines · Andi Kleen

put_page_dirty must use the page protection of the stack VMA, not hardcoded PAGE_COPY. They can be different e.g. when the stack is set non executable via VM_STACK_FLAGS.
I added an fallback path for now because I'm not sure if the stack VMA is always predowngrowed here, if the printk better it may be also needed to add an stack extension here.

We know which vma is involved at the callsite and what we just set its vma->vm_page_prot to; I suggest this patch instead.

-- wli

exec.c renamed from fs/exec.c +1 -1
diff -prauN linux-2.5.69-1/fs/exec.c exec-2.5.69-1/fs/exec.c
@@ -314,7 +314,7 @@
 	}
 	lru_cache_add_active(page);
 	flush_dcache_page(page);
-	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, PAGE_COPY))));
+	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, protection_map[VM_STACK_FLAGS & 0x7]))));
 	pte_chain = page_add_rmap(page, pte, pte_chain);
 	pte_unmap(pte);
 	tsk->mm->rss++;
author
#2 lore raw
Quoted 2 lines · William Lee Irwin III

We know which vma is involved at the callsite and what we just set its vma->vm_page_prot to; I suggest this patch instead.

No that won't work. On x86-64 VM_STACK_FLAGS looks at the current thread state if it's an 32bit or 64bit process, and that's not decided at that point yet.

-Andi

#3 lore raw
In reply to Andi Kleen
Quoted 4 lines · Andi Kleen

put_page_dirty must use the page protection of the stack VMA, not hardcoded
PAGE_COPY. They can be different e.g. when the stack is set non executable via VM_STACK_FLAGS.

OK. It seems a bit inefficient to go looking up the vma immediately after having created it.

How about we simply pass the desired protection in to put_dirty_page()?

 arch/ia64/ia32/binfmt_elf32.c  |    4 ++--
 arch/s390/kernel/compat_exec.c |    5 ++---
 arch/x86_64/ia32/ia32_binfmt.c |    6 ++----
 fs/exec.c                      |   13 +++++++------
 include/linux/mm.h             |    3 ++-
 5 files changed, 15 insertions(+), 16 deletions(-)
arch/ia64/ia32/binfmt_elf32.c renamed from arch/ia64/ia32/binfmt_elf32.c~put_dirty_page-protection-fix +2 -2
diff -puN arch/ia64/ia32/binfmt_elf32.c~put_dirty_page-protection-fix arch/ia64/ia32/binfmt_elf32.c
@@ -12,6 +12,7 @@
 #include <linux/config.h>
 
 #include <linux/types.h>
+#include <linux/mm.h>
 
 #include <asm/param.h>
 #include <asm/signal.h>

@@ -40,7 +41,6 @@
 #define CLOCKS_PER_SEC	IA32_CLOCKS_PER_SEC
 
 extern void ia64_elf32_init (struct pt_regs *regs);
-extern void put_dirty_page (struct task_struct * tsk, struct page *page, unsigned long address);
 
 static void elf32_set_personality (void);
 

@@ -200,7 +200,7 @@ ia32_setup_arg_pages (struct linux_binpr
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current, page, stack_base);
+			put_dirty_page(current, page, stack_base, PAGE_COPY);
 		}
 		stack_base += PAGE_SIZE;
 	}

diff -puN arch/ia64/mm/init.c~put_dirty_page-protection-fix arch/ia64/mm/init.c

arch/s390/kernel/compat_exec.c renamed from arch/s390/kernel/compat_exec.c~put_dirty_page-protection-fix +2 -3
diff -puN arch/s390/kernel/compat_exec.c~put_dirty_page-protection-fix arch/s390/kernel/compat_exec.c
@@ -18,6 +18,7 @@
 #include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
+#include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
 #include <linux/binfmts.h>

@@ -32,8 +33,6 @@
 #endif
 
 
-extern void put_dirty_page(struct task_struct * tsk, struct page *page, unsigned long address);
-
 #undef STACK_TOP
 #define STACK_TOP TASK31_SIZE
 

@@ -81,7 +80,7 @@ int setup_arg_pages32(struct linux_binpr
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base);
+			put_dirty_page(current,page,stack_base,PAGE_COPY);
 		}
 		stack_base += PAGE_SIZE;
 	}
arch/x86_64/ia32/ia32_binfmt.c renamed from arch/x86_64/ia32/ia32_binfmt.c~put_dirty_page-protection-fix +2 -4
diff -puN arch/x86_64/ia32/ia32_binfmt.c~put_dirty_page-protection-fix arch/x86_64/ia32/ia32_binfmt.c
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/string.h>
 #include <linux/binfmts.h>
+#include <linux/mm.h>
 #include <asm/segment.h> 
 #include <asm/ptrace.h>
 #include <asm/processor.h>

@@ -272,9 +273,6 @@ static void elf32_init(struct pt_regs *r
 	set_thread_flag(TIF_IA32); 
 }
 
-extern void put_dirty_page(struct task_struct * tsk, struct page *page, unsigned long address);
- 
-
 int setup_arg_pages(struct linux_binprm *bprm)
 {
 	unsigned long stack_base;

@@ -319,7 +317,7 @@ int setup_arg_pages(struct linux_binprm 
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base);
+			put_dirty_page(current,page,stack_base,PAGE_COPY_EXEC);
 		}
 		stack_base += PAGE_SIZE;
 	}
fs/exec.c renamed from fs/exec.c~put_dirty_page-protection-fix +7 -6
diff -puN fs/exec.c~put_dirty_page-protection-fix fs/exec.c
@@ -287,7 +287,8 @@ int copy_strings_kernel(int argc,char **
  *
  * tsk->mmap_sem is held for writing.
  */
-void put_dirty_page(struct task_struct * tsk, struct page *page, unsigned long address)
+void put_dirty_page(struct task_struct *tsk, struct page *page,
+			unsigned long address, pgprot_t prot)
 {
 	pgd_t * pgd;
 	pmd_t * pmd;

@@ -295,7 +296,8 @@ void put_dirty_page(struct task_struct *
 	struct pte_chain *pte_chain;
 
 	if (page_count(page) != 1)
-		printk(KERN_ERR "mem_map disagrees with %p at %08lx\n", page, address);
+		printk(KERN_ERR "mem_map disagrees with %p at %08lx\n",
+				page, address);
 
 	pgd = pgd_offset(tsk->mm, address);
 	pte_chain = pte_chain_alloc(GFP_KERNEL);

@@ -314,7 +316,7 @@ void put_dirty_page(struct task_struct *
 	}
 	lru_cache_add_active(page);
 	flush_dcache_page(page);
-	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, PAGE_COPY))));
+	set_pte(pte, pte_mkdirty(pte_mkwrite(mk_pte(page, prot))));
 	pte_chain = page_add_rmap(page, pte, pte_chain);
 	pte_unmap(pte);
 	tsk->mm->rss++;

@@ -421,7 +423,8 @@ int setup_arg_pages(struct linux_binprm 
 		struct page *page = bprm->page[i];
 		if (page) {
 			bprm->page[i] = NULL;
-			put_dirty_page(current,page,stack_base);
+			put_dirty_page(current, page, stack_base,
+					mpnt->vm_page_prot);
 		}
 		stack_base += PAGE_SIZE;
 	}

@@ -434,8 +437,6 @@ int setup_arg_pages(struct linux_binprm 
 
 #else
 
-#define put_dirty_page(tsk, page, address)
-#define setup_arg_pages(bprm)			(0)
 static inline void free_arg_pages(struct linux_binprm *bprm)
 {
 	int i;
include/linux/mm.h renamed from include/linux/mm.h~put_dirty_page-protection-fix +2 -1
diff -puN include/linux/mm.h~put_dirty_page-protection-fix include/linux/mm.h
@@ -421,7 +421,8 @@ extern int handle_mm_fault(struct mm_str
 extern int make_pages_present(unsigned long addr, unsigned long end);
 extern int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write);
 extern long sys_remap_file_pages(unsigned long start, unsigned long size, unsigned long prot, unsigned long pgoff, unsigned long nonblock);
-
+void put_dirty_page(struct task_struct *tsk, struct page *page,
+			unsigned long address, pgprot_t prot);
 
 int get_user_pages(struct task_struct *tsk, struct mm_struct *mm, unsigned long start,
 		int len, int write, int force, struct page **pages, struct vm_area_struct **vmas);

_

author
#4 lore raw
Quoted 10 lines · Andrew Morton
Quoted 4 lines · Andi Kleen

put_page_dirty must use the page protection of the stack VMA, not hardcoded
PAGE_COPY. They can be different e.g. when the stack is set non executable via VM_STACK_FLAGS.

OK. It seems a bit inefficient to go looking up the vma immediately after having created it.

How about we simply pass the desired protection in to put_dirty_page()?

Fine by me.

-Andi