- maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

- maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[akpm]

The patch titled
     maps2-add-proc-pid-pagemap-interface fix
has been removed from the -mm tree.  Its filename was
     maps2-add-proc-pid-pagemap-interface-fix.patch

This patch was dropped because it was folded into maps2-add-proc-pid-pagemap-interface.patch

------------------------------------------------------
Subject: maps2-add-proc-pid-pagemap-interface fix
From: Matt Mackall <mpm@selenic.com>

When CONFIG_HIGHPTE is enabled, use double-buffering in pagemap to
avoid calling copy_to_user while preemption is disabled.

Tested on x86 with HIGHPTE with DEBUG_SPINLOCK_SLEEP and
PROVE_LOCKING.

Signed-off-by: Matt Mackall <mpm@selenic.com>
Cc: Alexey Dobriyan <adobriyan@sw.ru>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/proc/task_mmu.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

diff -puN fs/proc/task_mmu.c~maps2-add-proc-pid-pagemap-interface-fix fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c~maps2-add-proc-pid-pagemap-interface-fix
+++ a/fs/proc/task_mmu.c
@@ -520,6 +520,7 @@ struct pagemapread {
 	struct mm_struct *mm;
 	unsigned long next;
 	unsigned long *buf;
+	pte_t *ptebuf;
 	unsigned long pos;
 	size_t count;
 	int index;
@@ -558,6 +559,14 @@ static int pagemap_pte_range(pmd_t *pmd,
 	int err;
 
 	pte = pte_offset_map(pmd, addr);
+
+#ifdef CONFIG_HIGHPTE
+	/* copy PTE directory to temporary buffer and unmap it */
+	memcpy(pm->ptebuf, pte, PAGE_ALIGN((unsigned long)pte) - (unsigned long)pte);
+	pte_unmap(pte);
+	pte = pm->ptebuf;
+#endif
+
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
 		if (addr < pm->next)
 			continue;
@@ -568,7 +577,11 @@ static int pagemap_pte_range(pmd_t *pmd,
 		if (err)
 			return err;
 	}
+
+#ifndef CONFIG_HIGHPTE
 	pte_unmap(pte - 1);
+#endif
+
 	return 0;
 }
 
@@ -640,10 +653,16 @@ static ssize_t pagemap_read(struct file 
 	if (!page)
 		goto out;
 
+#ifdef CONFIG_HIGHPTE
+	pm.ptebuf = kzalloc(PAGE_SIZE, GFP_USER);
+	if (!pm.ptebuf)
+		goto out_free;
+#endif
+
 	ret = 0;
 	mm = get_task_mm(task);
 	if (!mm)
-		goto out_free;
+		goto out_freepte;
 
 	pm.mm = mm;
 	pm.next = addr;
@@ -666,7 +685,7 @@ static ssize_t pagemap_read(struct file 
 	while (pm.count > 0 && vma) {
 		if (!ptrace_may_attach(task)) {
 			ret = -EIO;
-			goto out;
+			goto out_mm;
 		}
 		vend = min(vma->vm_start - 1, end - 1) + 1;
 		ret = pagemap_fill(&pm, vend);
@@ -685,8 +704,13 @@ static ssize_t pagemap_read(struct file 
 	if (!ret)
 		ret = pm.pos - src;
 
+out_mm:
 	mmput(mm);
+out_freepte:
+#ifdef CONFIG_HIGHPTE
+	kfree(pm.ptebuf);
 out_free:
+#endif
 	kfree(page);
 out:
 	put_task_struct(task);
_

Patches currently in -mm which might be from mpm@selenic.com are

slab-introduce-krealloc.patch
smaps-add-clear_refs-file-to-clear-reference.patch
maps2-uninline-some-functions-in-the-page-walker.patch
maps2-eliminate-the-pmd_walker-struct-in-the-page-walker.patch
maps2-remove-vma-from-args-in-the-page-walker.patch
maps2-propagate-errors-from-callback-in-page-walker.patch
maps2-add-callbacks-for-each-level-to-page-walker.patch
maps2-move-the-page-walker-code-to-lib.patch
maps2-simplify-interdependence-of-proc-pid-maps-and-smaps.patch
maps2-move-clear_refs-code-to-task_mmuc.patch
maps2-regroup-task_mmu-by-interface.patch
maps2-make-proc-pid-smaps-optional-under-config_embedded.patch
maps2-make-proc-pid-clear_refs-option-under-config_embedded.patch
maps2-add-proc-pid-pagemap-interface.patch
maps2-add-proc-pid-pagemap-interface-fix.patch
maps2-add-proc-pid-pagemap-interface-fix-fix.patch
maps2-add-proc-kpagemap-interface.patch
maps2-add-proc-kpagemap-interface-fix.patch
slob-handle-slab_panic-flag.patch

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Alexey Dobriyan]

> maps2-add-proc-pid-pagemap-interface.patch

Ohhh, you're repeating december mincore() bug
2f77d107050abc14bc393b34bdb7b91cf670c250

pagemap_read() takes ->mmap_sem for reading
walk_page_range
pagemap_pte_range
add_to_pagemap
flush_pagemap
copy_to_user

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Andrew Morton]

On Fri, 27 Apr 2007 14:45:33 +0400
Alexey Dobriyan <adobriyan@sw.ru> wrote:

> > maps2-add-proc-pid-pagemap-interface.patch
> 
> Ohhh, you're repeating december mincore() bug
> 2f77d107050abc14bc393b34bdb7b91cf670c250
> 
> pagemap_read() takes ->mmap_sem for reading
> walk_page_range
> pagemap_pte_range
> add_to_pagemap
> flush_pagemap
> copy_to_user

argh.  I think it's always a bug to run uaccess functions while holding
mmap_sem, isn't it?

I'll see if I can get something like this working as a -mm-only thing:

diff -puN include/asm-i386/uaccess.h~i386-uaccess-debugging include/asm-i386/uaccess.h
--- a/include/asm-i386/uaccess.h~i386-uaccess-debugging
+++ a/include/asm-i386/uaccess.h
@@ -33,6 +33,8 @@
 
 #define segment_eq(a,b)	((a).seg == (b).seg)
 
+void no_mmap_sem(void);
+
 /*
  * movsl can be slow when source and dest are not both 8-byte aligned
  */
@@ -149,6 +151,7 @@ extern void __get_user_4(void);
 ({	int __ret_gu;							\
 	unsigned long __val_gu;						\
 	__chk_user_ptr(ptr);						\
+	no_mmap_sem();							\
 	switch(sizeof (*(ptr))) {					\
 	case 1:  __get_user_x(1,__ret_gu,__val_gu,ptr); break;		\
 	case 2:  __get_user_x(2,__ret_gu,__val_gu,ptr); break;		\
@@ -198,6 +201,7 @@ extern void __put_user_8(void);
 ({	int __ret_pu;						\
 	__typeof__(*(ptr)) __pu_val;				\
 	__chk_user_ptr(ptr);					\
+	no_mmap_sem();						\
 	__pu_val = x;						\
 	switch(sizeof(*(ptr))) {				\
 	case 1: __put_user_1(__pu_val, ptr); break;		\
@@ -215,6 +219,7 @@ extern void __put_user_8(void);
  	int __ret_pu;						\
 	__typeof__(*(ptr)) __pus_tmp = x;			\
 	__ret_pu=0;						\
+	no+_mmap_sem();						\
 	if(unlikely(__copy_to_user_ll(ptr, &__pus_tmp,		\
 				sizeof(*(ptr))) != 0))		\
  		__ret_pu=-EFAULT;				\
@@ -301,6 +306,7 @@ extern void __put_user_8(void);
 do {									\
 	retval = 0;							\
 	__chk_user_ptr(ptr);						\
+	no_mmap_sem();							\
 	switch (size) {							\
 	case 1: __put_user_asm(x,ptr,retval,"b","b","iq",errret);break;	\
 	case 2: __put_user_asm(x,ptr,retval,"w","w","ir",errret);break; \
@@ -316,6 +322,7 @@ do {									\
 do {									\
 	__typeof__(*(ptr)) __pus_tmp = x;				\
 	retval = 0;							\
+	no_mmap_sem();							\
 									\
 	if(unlikely(__copy_to_user_ll(ptr, &__pus_tmp, size) != 0))	\
 		retval = errret;					\
@@ -361,6 +368,7 @@ extern long __get_user_bad(void);
 do {									\
 	retval = 0;							\
 	__chk_user_ptr(ptr);						\
+	no_mmap_sem();							\
 	switch (size) {							\
 	case 1: __get_user_asm(x,ptr,retval,"b","b","=q",errret);break;	\
 	case 2: __get_user_asm(x,ptr,retval,"w","w","=r",errret);break;	\
@@ -407,6 +415,7 @@ unsigned long __must_check __copy_from_u
 static __always_inline unsigned long __must_check
 __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
 {
+	no_mmap_sem();
 	if (__builtin_constant_p(n)) {
 		unsigned long ret;
 
@@ -454,6 +463,7 @@ __copy_from_user_inatomic(void *to, cons
 	 * but as the zeroing behaviour is only significant when n is not
 	 * constant, that shouldn't be a problem.
 	 */
+	no_mmap_sem();
 	if (__builtin_constant_p(n)) {
 		unsigned long ret;
 
diff -puN arch/i386/lib/usercopy.c~i386-uaccess-debugging arch/i386/lib/usercopy.c
--- a/arch/i386/lib/usercopy.c~i386-uaccess-debugging
+++ a/arch/i386/lib/usercopy.c
@@ -717,6 +717,7 @@ unsigned long __copy_to_user_ll(void __u
 				unsigned long n)
 {
 	BUG_ON((long) n < 0);
+	no_mmap_sem();
 #ifndef CONFIG_X86_WP_WORKS_OK
 	if (unlikely(boot_cpu_data.wp_works_ok == 0) &&
 			((unsigned long )to) < TASK_SIZE) {
@@ -786,6 +787,7 @@ unsigned long __copy_from_user_ll(void *
 					unsigned long n)
 {
 	BUG_ON((long)n < 0);
+	no_mmap_sem();
 	if (movsl_is_ok(to, from, n))
 		__copy_user_zeroing(to, from, n);
 	else
@@ -798,6 +800,7 @@ unsigned long __copy_from_user_ll_nozero
 					 unsigned long n)
 {
 	BUG_ON((long)n < 0);
+	no_mmap_sem();
 	if (movsl_is_ok(to, from, n))
 		__copy_user(to, from, n);
 	else
@@ -811,6 +814,7 @@ unsigned long __copy_from_user_ll_nocach
 					unsigned long n)
 {
 	BUG_ON((long)n < 0);
+	no_mmap_sem();
 #ifdef CONFIG_X86_INTEL_USERCOPY
 	if ( n > 64 && cpu_has_xmm2)
                 n = __copy_user_zeroing_intel_nocache(to, from, n);
@@ -826,6 +830,7 @@ unsigned long __copy_from_user_ll_nocach
 					unsigned long n)
 {
 	BUG_ON((long)n < 0);
+	no_mmap_sem();
 #ifdef CONFIG_X86_INTEL_USERCOPY
 	if ( n > 64 && cpu_has_xmm2)
                 n = __copy_user_intel_nocache(to, from, n);
@@ -887,3 +892,16 @@ copy_from_user(void *to, const void __us
 	return n;
 }
 EXPORT_SYMBOL(copy_from_user);
+
+void no_mmap_sem(void)
+{
+	struct mm_struct *mm;
+
+	if (in_atomic())
+		return;		/* We won't take pagefaults */
+	mm = current->mm;
+	if (!mm)
+		return;
+	WARN_ON(rwsem_is_locked(&mm->mmap_sem))
+}
+EXPORT_SYMBOL(no_mmap_sem);
_

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Matt Mackall]

On Fri, Apr 27, 2007 at 01:27:13PM -0700, Andrew Morton wrote:
> On Fri, 27 Apr 2007 14:45:33 +0400
> Alexey Dobriyan <adobriyan@sw.ru> wrote:
> 
> > > maps2-add-proc-pid-pagemap-interface.patch
> > 
> > Ohhh, you're repeating december mincore() bug
> > 2f77d107050abc14bc393b34bdb7b91cf670c250
> > 
> > pagemap_read() takes ->mmap_sem for reading
> > walk_page_range
> > pagemap_pte_range
> > add_to_pagemap
> > flush_pagemap
> > copy_to_user
> 
> argh.  I think it's always a bug to run uaccess functions while holding
> mmap_sem, isn't it?

Yep, looks that way.

> I'll see if I can get something like this working as a -mm-only thing:

I was wondering if there was a sensible way to do this. This looks
pretty good.
 
> +	no+_mmap_sem();						\

Except for this bit. Maybe put it in the same config bucket as might_sleep?

-- 
Mathematics is the supreme nostalgia of our time.

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Andrew Morton]

On Fri, 27 Apr 2007 15:41:55 -0500
Matt Mackall <mpm@selenic.com> wrote:

> > +	no+_mmap_sem();						\
> 
> Except for this bit.

You'll put gcc out of a job ;)

> Maybe put it in the same config bucket as might_sleep?

hm, could do.  might_sleep() is intertwined with preempt in complex ways,
but we did decouple that at the config level.  no_mmap_sem() will dtrt for
all preempt settings.

But I'll be keeping this as a -mm-only debug patch (which brings us up to
about thirty of 'em), so I think it's best to make it unconfigurable so we
get maximum coverage.

That's if it actually works.  I haven't tried running it yet, and I have a
feeling that running it might cause a big "doh" moment.  We'll see.

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Hugh Dickins]

On Fri, 27 Apr 2007, Andrew Morton wrote:
> 
> hm, could do.  might_sleep() is intertwined with preempt in complex ways,
> but we did decouple that at the config level.  no_mmap_sem() will dtrt for
> all preempt settings.
> 
> But I'll be keeping this as a -mm-only debug patch (which brings us up to
> about thirty of 'em), so I think it's best to make it unconfigurable so we
> get maximum coverage.
> 
> That's if it actually works.  I haven't tried running it yet, and I have a
> feeling that running it might cause a big "doh" moment.  We'll see.

Yes, I'm expecting the crucial

> +	WARN_ON(rwsem_is_locked(&mm->mmap_sem))

to give a bogus warning every time another thread (or /proc,
or swapoff, or whatever) happens to have this mmap_sem locked.
might_sleep() is quite different, works on our thread's info.

Hugh

Re: - maps2-add-proc-pid-pagemap-interface-fix.patch removed from -mm tree

[Andrew Morton]

On Sat, 28 Apr 2007 06:13:39 +0100 (BST) Hugh Dickins <hugh@veritas.com> wrote:

> On Fri, 27 Apr 2007, Andrew Morton wrote:
> > 
> > hm, could do.  might_sleep() is intertwined with preempt in complex ways,
> > but we did decouple that at the config level.  no_mmap_sem() will dtrt for
> > all preempt settings.
> > 
> > But I'll be keeping this as a -mm-only debug patch (which brings us up to
> > about thirty of 'em), so I think it's best to make it unconfigurable so we
> > get maximum coverage.
> > 
> > That's if it actually works.  I haven't tried running it yet, and I have a
> > feeling that running it might cause a big "doh" moment.  We'll see.
> 
> Yes, I'm expecting the crucial
> 
> > +	WARN_ON(rwsem_is_locked(&mm->mmap_sem))
> 
> to give a bogus warning every time another thread (or /proc,
> or swapoff, or whatever) happens to have this mmap_sem locked.
> might_sleep() is quite different, works on our thread's info.
> 

Yes.  lockdep has a way of working out if this task already has a
particular lock for reading or writing, but it isn't immediately obvious
how to extract that.

I guess a simple hack would be do do a down_read() on it.  If it's already
held for reading, lockdep should warn.  If it's already held for writing
someone will notice.

Oh well, it's not my top priority.