mpol_to_str revisited.

Last month I sent in 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a to remove a user triggerable BUG in mempolicy.

Ben Hutchings pointed out to me that my change introduced a potential leak of stack contents to userspace, because none of the callers check the return value.

This patch adds the missing return checking, and also clears the buffer beforehand.

Reported-by Ben Hutchings <[email protected]>

Cc [email protected]

Signed-off-by Dave Jones <[email protected]>

---
unanswered question: why are the buffer sizes here different ? which is correct?

If pol->mode was poisoned, that smells like we have a race where policy is getting freed while another process is reading it.

Am I missing something, or is there no locking around that at all ?

unanswered question: why are the buffer sizes here different ? which is correct?

diff -durpN '--exclude-from=/home/davej/.exclude' src/git-trees/kernel/linux/fs/proc/task_mmu.c linux-dj/fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +1 -0

@@ -1162,6 +1162,7 @@ static int show_numa_map(struct seq_file
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
 	int n;
+	int ret;
 	char buffer[50];
 
 	if (!mm)

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +1 -0

+
 	mpol_cond_put(pol);
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);

diff -durpN '--exclude-from=/home/davej/.exclude' src/git-trees/kernel/linux/mm/shmem.c linux-dj/mm/shmem.c

mm/shmem.c renamed from git-trees/kernel/linux/mm/shmem.c +1 -0

@@ -885,13 +885,15 @@ redirty:
 static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
 		return;		/* show nothing */
 

Quoted 2 lines

unanswered question: why are the buffer sizes here different ? which is correct?

Given the current set of mempolicy modes and flags, it's 34, but this can change if new modes or flags are added with longer names. I see no reason why shmem shouldn't round up to the nearest power-of-2 of 64 like it already does, but 50 is certainly safe as well in task_mmu.c.

Quoted 14 lines

diff -durpN '--exclude-from=/home/davej/.exclude' src/git-trees/kernel/linux/fs/proc/task_mmu.c linux-dj/fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +1 -0

@@ -1162,6 +1162,7 @@ static int show_numa_map(struct seq_file
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
 	int n;
+	int ret;
 	char buffer[50];
 
 	if (!mm)

@@ -1178,7 +1179,11 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 

fs/proc/task_mmu.c static int show_numa_map(struct seq_file · line 1185

Quoted 6 lines

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +4 -1

 	pol = get_vma_policy(proc_priv->task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol, 0);
+	memset(buffer, 0, sizeof(buffer));
+	ret = mpol_to_str(buffer, sizeof(buffer), pol, 0);
+	if (ret < 0)
+		return 0;

We should need the mpol_cond_put(pol) here before returning.

Quoted 14 lines

Quoted 14 lines

diff -durpN '--exclude-from=/home/davej/.exclude' src/git-trees/kernel/linux/fs/proc/task_mmu.c linux-dj/fs/proc/task_mmu.c

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +1 -0

@@ -1162,6 +1162,7 @@ static int show_numa_map(struct seq_file
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
 	int n;
+	int ret;
 	char buffer[50];
 
 	if (!mm)

@@ -1178,7 +1179,11 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 

Quoted 8 lines

fs/proc/task_mmu.c static int show_numa_map(struct seq_file · line 1185

Quoted 6 lines

fs/proc/task_mmu.c renamed from git-trees/kernel/linux/fs/proc/task_mmu.c +4 -1

 	pol = get_vma_policy(proc_priv->task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol, 0);
+	memset(buffer, 0, sizeof(buffer));
+	ret = mpol_to_str(buffer, sizeof(buffer), pol, 0);
+	if (ret < 0)
+		return 0;

We should need the mpol_cond_put(pol) here before returning.

good catch. I'll respin the patch later with this changed.

Last month I sent in 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a to remove a user triggerable BUG in mempolicy.

Ben Hutchings pointed out to me that my change introduced a potential leak of stack contents to userspace, because none of the callers check the return value.

This patch adds the missing return checking, and also clears the buffer beforehand.

Reported-by Ben Hutchings <[email protected]>

Cc [email protected]

Signed-off-by Dave Jones <[email protected]>

---
unanswered question: why are the buffer sizes here different ? which is correct?

Last month I sent in 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a to remove a user triggerable BUG in mempolicy.

Ben Hutchings pointed out to me that my change introduced a potential leak of stack contents to userspace, because none of the callers check the return value.

This patch adds the missing return checking, and also clears the buffer beforehand.

I don't think 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a is right fix.

we should
close a race (or kill remain ref count leak) if we still have.

Quoted 1 line · KOSAKI Motohiro

I don't think 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a is right fix.

It's certainly not a complete fix, but I think it's a much better result of the race, i.e. we don't panic anymore, we simply fail the read() instead.

Quoted 2 lines

we should
close a race (or kill remain ref count leak) if we still have.

As I mentioned earlier in the thread, the read() is done here on a task while only a reference to the task_struct is taken and we do not hold task_lock() which is required for task->mempolicy. Once that is fixed, mpol_to_str() should never be called for !task->mempolicy so it will never need to return -EINVAL in such a condition.

Quoted 5 lines

Quoted 1 line

I don't think 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a is right fix.

It's certainly not a complete fix, but I think it's a much better result of the race, i.e. we don't panic anymore, we simply fail the read() instead.

Even though 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a itself is simple. It bring to caller complex. That's not good and have no worth.

Quoted 9 lines · KOSAKI Motohiro

Quoted 5 lines

Quoted 1 line

I don't think 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a is right fix.

It's certainly not a complete fix, but I think it's a much better result of the race, i.e. we don't panic anymore, we simply fail the read() instead.

Even though 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a itself is simple. It bring to caller complex. That's not good and have no worth.

Before: the kernel panics, all workloads cease.
After: the file shows garbage, all workloads continue.

This is better, in my opinion, but at best it's only a judgment call and has no effect on anything.

I agree it would be better to respect the return value of mpol_to_str() since there are other possible error conditions other than a freed mempolicy, but let's not consider reverting 80de7c3138. It is obviously not a full solution to the problem, though, and we need to serialize with task_lock().

Dave, are you interested in coming up with a patch?

Quoted 4 lines

Quoted 3 lines

Even though 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a itself is simple. It bring to caller complex. That's not good and have no worth.

Quoted 5 lines

Before: the kernel panics, all workloads cease.
After: the file shows garbage, all workloads continue.

This is better, in my opinion, but at best it's only a judgment call and has no effect on anything.

Kernel panics help to find our serious mistake.

Quoted 5 lines

I agree it would be better to respect the return value of mpol_to_str() since there are other possible error conditions other than a freed mempolicy, but let's not consider reverting 80de7c3138. It is obviously not a full solution to the problem, though, and we need to serialize with task_lock().

Sorry no. I will have to revert it.

When reading /proc/pid/numa_maps, it's possible to return the contents of the stack where the mempolicy string should be printed if the policy gets freed from beneath us.

This happens because mpol_to_str() may return an error the stack-allocated buffer is then printed without ever being stored.

There are two possible error conditions in mpol_to_str():

- if the buffer allocated is insufficient for the string to be stored, and

- if the mempolicy has an invalid mode.

The first error condition is not triggered in any of the callers to mpol_to_str(): at least 50 bytes is always allocated on the stack and this is sufficient for the string to be written. A future patch should convert this into BUILD_BUG_ON() since we know the maximum strlen possible, but that's not -rc material.

The second error condition is possible if a race occurs in dropping a reference to a task's mempolicy causing it to be freed during the read(). The slab poison value is then used for the mode and mpol_to_str() returns -EINVAL.

This race is only possible because get_vma_policy() believes that mm->mmap_sem protects task->mempolicy, which isn't true. The exit path does not hold mm->mmap_sem when dropping the reference or setting task->mempolicy to NULL: it uses task_lock(task) instead.

Thus, it's required for the caller of a task mempolicy to hold task_lock(task) while grabbing the mempolicy and reading it. Callers with a vma policy store their mempolicy earlier and can simply increment the reference count so it's guaranteed not to be freed.

Reported-by Dave Jones <[email protected]>

Signed-off-by David Rientjes <[email protected]>

---
 fs/proc/task_mmu.c |    7 +++++--
 mm/mempolicy.c     |    5 ++---
 2 files changed, 7 insertions(+), 5 deletions(-)

fs/proc/task_mmu.c +5 -2

@@ -1158,6 +1158,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
        struct vm_area_struct *vma = v;
        struct numa_maps *md = &numa_priv->md;
        struct file *file = vma->vm_file;
+       struct task_struct *task = proc_priv->task;
        struct mm_struct *mm = vma->vm_mm;
        struct mm_walk walk = {};
        struct mempolicy *pol;

@@ -1177,9 +1178,11 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
        walk.private = md;
        walk.mm = mm;
 
-       pol = get_vma_policy(proc_priv->task, vma, vma->vm_start);
+       task_lock(task);
+       pol = get_vma_policy(task, vma, vma->vm_start);
        mpol_to_str(buffer, sizeof(buffer), pol, 0);
        mpol_cond_put(pol);
+       task_unlock(task);
 
        seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

@@ -1189,7 +1192,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
        } else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
                seq_printf(m, " heap");
        } else {
-               pid_t tid = vm_is_stack(proc_priv->task, vma, is_pid);
+               pid_t tid = vm_is_stack(task, vma, is_pid);
                if (tid != 0) {
                        /*
                         * Thread stack in /proc/PID/task/TID/maps or

mm/mempolicy.c +0 -0

index 0b78fb9..d04a8a5 100644

Quoted 9 lines

mm/mempolicy.c +0 -1

index 0b78fb9..d04a8a5 100644
@@ -1536,9 +1536,8 @@ asmlinkage long compat_sys_mbind(compat_ulong_t start, compat_ulong_t len,
  *
  * Returns effective policy for a VMA at specified address.
  * Falls back to @task or system default policy, as necessary.
- * Current or other task's task mempolicy and non-shared vma policies

mm/mempolicy.c asmlinkage long compat_sys_mbind(compat_ulong_t start, compat_ulong_t len, · line 1540

Quoted 4 lines

mm/mempolicy.c +2 -2

- * are protected by the task's mmap_sem, which must be held for read by
- * the caller.
+ * Current or other task's task mempolicy and non-shared vma policies must be
+ * protected by task_lock(task) by the caller.

This is not correct. mmap_sem is needed for protecting vma. task_lock() is needed to close vs exit race only when task != current. In other word, caller must held both mmap_sem and task_lock if task != current.

Quoted 9 lines · KOSAKI Motohiro

Quoted 9 lines

mm/mempolicy.c +0 -1

index 0b78fb9..d04a8a5 100644
@@ -1536,9 +1536,8 @@ asmlinkage long compat_sys_mbind(compat_ulong_t start, compat_ulong_t len,
  *
  * Returns effective policy for a VMA at specified address.
  * Falls back to @task or system default policy, as necessary.
- * Current or other task's task mempolicy and non-shared vma policies

Quoted 8 lines

mm/mempolicy.c asmlinkage long compat_sys_mbind(compat_ulong_t start, compat_ulong_t len, · line 1540

Quoted 4 lines

mm/mempolicy.c +2 -2

- * are protected by the task's mmap_sem, which must be held for read by
- * the caller.
+ * Current or other task's task mempolicy and non-shared vma policies must be
+ * protected by task_lock(task) by the caller.

This is not correct. mmap_sem is needed for protecting vma. task_lock() is needed to close vs exit race only when task != current. In other word, caller must held both mmap_sem and task_lock if task != current.

The comment is specifically addressing non-shared vma policies, you do not need to hold mmap_sem to access another thread's mempolicy.

BUG: sleeping function called from invalid context at kernel/mutex.c:269
in_atomic(): 1, irqs_disabled(): 0, pid: 8558, name: trinity-child2
3 locks on stack by trinity-child2/8558:
 #0: held:     (&p->lock){+.+.+.}, instance: ffff88010c9a00b0, at: [<ffffffff8120cd1f>] seq_lseek+0x3f/0x120
 #1: held:     (&mm->mmap_sem){++++++}, instance: ffff88013956f7c8, at: [<ffffffff81254437>] m_start+0xa7/0x190
 #2: held:     (&(&p->alloc_lock)->rlock){+.+...}, instance: ffff88011fc64f30, at: [<ffffffff81254f8f>] show_numa_map+0x14f/0x610
Pid: 8558, comm: trinity-child2 Not tainted 3.7.0-rc1+ #32
Call Trace:
 [<ffffffff810ae4ec>] __might_sleep+0x14c/0x200
 [<ffffffff816bdf4e>] mutex_lock_nested+0x2e/0x50
 [<ffffffff811c43a3>] mpol_shared_policy_lookup+0x33/0x90
 [<ffffffff8118d5c3>] shmem_get_policy+0x33/0x40
 [<ffffffff811c31fa>] get_vma_policy+0x3a/0x90

 [<ffffffff81254fa3>] show_numa_map+0x163/0x610
 [<ffffffff81255b10>] ? pid_maps_open+0x20/0x20
 [<ffffffff81255980>] ? pagemap_hugetlb_range+0xf0/0xf0
 [<ffffffff81255483>] show_pid_numa_map+0x13/0x20
 [<ffffffff8120c902>] traverse+0xf2/0x230
 [<ffffffff8120cd8b>] seq_lseek+0xab/0x120
 [<ffffffff811e6c0b>] sys_lseek+0x7b/0xb0
 [<ffffffff816ca088>] tracesys+0xe1/0xe6

Quoted 13 lines · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269
in_atomic(): 1, irqs_disabled(): 0, pid: 8558, name: trinity-child2
3 locks on stack by trinity-child2/8558:
  #0: held:     (&p->lock){+.+.+.}, instance: ffff88010c9a00b0, at: [<ffffffff8120cd1f>] seq_lseek+0x3f/0x120
  #1: held:     (&mm->mmap_sem){++++++}, instance: ffff88013956f7c8, at: [<ffffffff81254437>] m_start+0xa7/0x190
  #2: held:     (&(&p->alloc_lock)->rlock){+.+...}, instance: ffff88011fc64f30, at: [<ffffffff81254f8f>] show_numa_map+0x14f/0x610
Pid: 8558, comm: trinity-child2 Not tainted 3.7.0-rc1+ #32
Call Trace:
  [<ffffffff810ae4ec>] __might_sleep+0x14c/0x200
  [<ffffffff816bdf4e>] mutex_lock_nested+0x2e/0x50
  [<ffffffff811c43a3>] mpol_shared_policy_lookup+0x33/0x90
  [<ffffffff8118d5c3>] shmem_get_policy+0x33/0x40
  [<ffffffff811c31fa>] get_vma_policy+0x3a/0x90

Quoted 9 lines

  [<ffffffff81254fa3>] show_numa_map+0x163/0x610
  [<ffffffff81255b10>] ? pid_maps_open+0x20/0x20
  [<ffffffff81255980>] ? pagemap_hugetlb_range+0xf0/0xf0
  [<ffffffff81255483>] show_pid_numa_map+0x13/0x20
  [<ffffffff8120c902>] traverse+0xf2/0x230
  [<ffffffff8120cd8b>] seq_lseek+0xab/0x120
  [<ffffffff811e6c0b>] sys_lseek+0x7b/0xb0
  [<ffffffff816ca088>] tracesys+0xe1/0xe6

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Comments on this approach?

Quoted 34 lines · David Rientjes

Quoted 16 lines · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269
in_atomic(): 1, irqs_disabled(): 0, pid: 8558, name: trinity-child2
3 locks on stack by trinity-child2/8558:
  #0: held:     (&p->lock){+.+.+.}, instance: ffff88010c9a00b0, at:
[<ffffffff8120cd1f>] seq_lseek+0x3f/0x120
  #1: held:     (&mm->mmap_sem){++++++}, instance: ffff88013956f7c8, at:
[<ffffffff81254437>] m_start+0xa7/0x190
  #2: held:     (&(&p->alloc_lock)->rlock){+.+...}, instance:
ffff88011fc64f30, at: [<ffffffff81254f8f>] show_numa_map+0x14f/0x610
Pid: 8558, comm: trinity-child2 Not tainted 3.7.0-rc1+ #32
Call Trace:
  [<ffffffff810ae4ec>] __might_sleep+0x14c/0x200
  [<ffffffff816bdf4e>] mutex_lock_nested+0x2e/0x50
  [<ffffffff811c43a3>] mpol_shared_policy_lookup+0x33/0x90
  [<ffffffff8118d5c3>] shmem_get_policy+0x33/0x40
  [<ffffffff811c31fa>] get_vma_policy+0x3a/0x90

Quoted 9 lines

  [<ffffffff81254fa3>] show_numa_map+0x163/0x610
  [<ffffffff81255b10>] ? pid_maps_open+0x20/0x20
  [<ffffffff81255980>] ? pagemap_hugetlb_range+0xf0/0xf0
  [<ffffffff81255483>] show_pid_numa_map+0x13/0x20
  [<ffffffff8120c902>] traverse+0xf2/0x230
  [<ffffffff8120cd8b>] seq_lseek+0xab/0x120
  [<ffffffff811e6c0b>] sys_lseek+0x7b/0xb0
  [<ffffffff816ca088>] tracesys+0xe1/0xe6

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Quoted 1 line

Comments on this approach?

I think this refcounting is better than using task_lock().

Quoted 1 line

I think this refcounting is better than using task_lock().

I don't think so. get_vma_policy() is used from fast path. In other words, number of
atomic ops is sensible for allocation performance.

Instead, I'd like
to use spinlock
for shared mempolicy instead of mutex.

Quoted 5 lines · KOSAKI Motohiro

Quoted 1 line

I think this refcounting is better than using task_lock().

I don't think so. get_vma_policy() is used from fast path. In other words, number of
atomic ops is sensible for allocation performance.

There are enhancements that we can make with refcounting: for instance, we may want to avoid doing it in the super-fast path when the policy is default_policy and then just do

        if (mpol != &default_policy)
                mpol_put(mpol);

Quoted 4 lines

Instead, I'd like
to use spinlock
for shared mempolicy instead of mutex.

Um, this was just changed to a mutex last week in commit b22d127a39dd ("mempolicy: fix a race in shared_policy_replace()") so that sp_alloc() can be done with GFP_KERNEL, so I didn't consider reverting that behavior. Are you nacking that patch, which you acked, now?

Quoted 4 lines

Um, this was just changed to a mutex last week in commit b22d127a39dd ("mempolicy: fix a race in shared_policy_replace()") so that sp_alloc() can be done with GFP_KERNEL, so I didn't consider reverting that behavior. Are you nacking that patch, which you acked, now?

Yes, sadly. /proc usage is a corner case issue. It's not worth to strike main path.

Quoted 1 line · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Comments on this approach?

Quoted 4 lines · David Rientjes

Quoted 1 line · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269

Quoted 6 lines

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Comments on this approach?

Seems to be surviving my testing at least..

Quoted 12 lines · Dave Jones

Quoted 10 lines · David Rientjes

Quoted 1 line · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Comments on this approach?

Quoted 2 lines

Seems to be surviving my testing at least..

Sounds good. Is it possible to verify that policy_cache isn't getting larger than normal in /proc/slabinfo, i.e. when all processes with a task mempolicy or shared vma policy have exited, are there still a significant number of active objects?

Quoted 4 lines

Sounds good. Is it possible to verify that policy_cache isn't getting larger than normal in /proc/slabinfo, i.e. when all processes with a task mempolicy or shared vma policy have exited, are there still a significant number of active objects?

Killing the fuzzer caused it to drop dramatically.

Before:

(15:29:59:davej@bitcrush:trinity[master])$ sudo cat /proc/slabinfo  | grep policy
shared_policy_node   2931   2967    376   43    4 : tunables    0    0    0 : slabdata     69     69      0
numa_policy         2971   6545    464   35    4 : tunables    0    0    0 : slabdata    187    187      0

After:

(15:30:16:davej@bitcrush:trinity[master])$ sudo cat /proc/slabinfo  | grep policy
shared_policy_node      0    215    376   43    4 : tunables    0    0    0 : slabdata      5      5      0
numa_policy           15    175    464   35    4 : tunables    0    0    0 : slabdata      5      5      0

Quoted 5 lines

Quoted 4 lines

Sounds good. Is it possible to verify that policy_cache isn't getting larger than normal in /proc/slabinfo, i.e. when all processes with a task mempolicy or shared vma policy have exited, are there still a significant number of active objects?

Quoted 2 lines

Killing the fuzzer caused it to drop dramatically.

Excellent, thanks. This shows that the refcounting is working properly and we're not leaking any references as a result of this change causing the mempolicies to never be freed. ("numa_policy" turns out to be policy_cache in the code, so thanks for checking both of them.)

Could I add your tested-by?

As a result of commit 32f8516a8c73 ("mm, mempolicy: fix printing stack contents in numa_maps"), the mutex protecting a shared policy can be inadvertently taken while holding task_lock(task).

Recently, commit b22d127a39dd ("mempolicy: fix a race in shared_policy_replace()") switched the spinlock within a shared policy to a mutex so sp_alloc() could block. Thus, a refcount must be grabbed on all mempolicies returned by get_vma_policy() so it isn't freed while being passed to mpol_to_str() when reading /proc/pid/numa_maps.

This patch only takes task_lock() while dereferencing task->mempolicy in get_vma_policy() if it's non-NULL in the lockess check to increment its refcount. This ensures it will remain in memory until dropped by __mpol_put() after mpol_to_str() is called.

Refcounts of shared policies are grabbed by the ->get_policy() function of the vma, all others will be grabbed directly in get_vma_policy(). Now that this is done, all callers now unconditionally drop the refcount.

When reading /proc/pid/numa_maps, it's possible to return the contents of the stack where the mempolicy string should be printed if the policy gets freed from beneath us.

This happens because mpol_to_str() may return an error the stack-allocated buffer is then printed without ever being stored.

fs/proc/task_mmu.c static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma) · line 103

Quoted 9 lines · Kamezawa Hiroyuki

         if (vma && vma != priv->tail_vma) {
                 struct mm_struct *mm = vma->vm_mm;

fs/proc/task_mmu.c +5 -0

+#ifdef CONFIG_NUMA
+               task_lock(priv->task);
+               __mpol_put(priv->task->mempolicy);
+               task_unlock(priv->task);
+#endif
                 up_read(&mm->mmap_sem);
                 mmput(mm);

Please don't put #ifdef's inside code. It makes things really ugly and hard to read.

And that is *especially* true in this case, since there's a pattern to all these things:

fs/proc/task_mmu.c static void *m_start(struct seq_file *m, loff_t *pos)

Quoted 11 lines

fs/proc/task_mmu.c +10 -0

+#ifdef CONFIG_NUMA
+       task_lock(priv->task);
+       mpol_get(priv->task->mempolicy);
+       task_unlock(priv->task);
+#endif
 
+#ifdef CONFIG_NUMA
+       task_lock(priv->task);
+       __mpol_put(priv->task->mempolicy);
+       task_unlock(priv->task);
+#endif

it really sounds like what you want to do is to just abstract a "numa_policy_get/put(priv)" operation.

So you could make it be something like

   #ifdef CONFIG_NUMA
   static inline numa_policy_get(struct proc_maps_private *priv)
   {
       task_lock(priv->task);
       mpol_get(priv->task->mempolicy);
       task_unlock(priv->task);
   }
   .. same for the "put" function ..
   #else
     #define numa_policy_get(priv) do { } while (0)
     #define numa_policy_put(priv) do { } while (0)
   #endif

and then you wouldn't have to have the #ifdef's in the middle of code, and I think it will be more readable in general.

Sure, it is going to be a few more actual lines of patch, but there's no duplicated code sequence, and the added lines are just the syntax that makes it look better.

Quoted 10 lines · David Rientjes

As a result of commit 32f8516a8c73 ("mm, mempolicy: fix printing stack contents in numa_maps"), the mutex protecting a shared policy can be inadvertently taken while holding task_lock(task).

Recently, commit b22d127a39dd ("mempolicy: fix a race in shared_policy_replace()") switched the spinlock within a shared policy to a mutex so sp_alloc() could block. Thus, a refcount must be grabbed on all mempolicies returned by get_vma_policy() so it isn't freed while being passed to mpol_to_str() when reading /proc/pid/numa_maps.

Quoted 9 lines

This patch only takes task_lock() while dereferencing task->mempolicy in get_vma_policy() if it's non-NULL in the lockess check to increment its refcount. This ensures it will remain in memory until dropped by __mpol_put() after mpol_to_str() is called.

Refcounts of shared policies are grabbed by the ->get_policy() function of the vma, all others will be grabbed directly in get_vma_policy(). Now that this is done, all callers now unconditionally drop the refcount.

please add original problem description....

from your 1st patch.

Quoted 6 lines

When reading /proc/pid/numa_maps, it's possible to return the contents of the stack where the mempolicy string should be printed if the policy gets freed from beneath us.

This happens because mpol_to_str() may return an error the stack-allocated buffer is then printed without ever being stored.

.....

Hmm, I've read the whole thread again...and, I'm sorry if I misunderstand something.

I think Kosaki mentioned the commit 52cd3b0740. It avoids refcounting in get_vma_policy() because it's called every time alloc_pages_vma() is called, at every page fault. So, it seems he doesn't agree this fix because of performance concern on big NUMA,

Can't we have another way to fix ? like this ? too ugly ? Again, I'm sorry if I misunderstand the points.

fs/proc/internal.h +4 -0

index cceaab0..43973b0 100644
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/proc_fs.h>
 struct  ctl_table_header;
+struct  mempolicy;
  extern struct proc_dir_entry proc_root;
 #ifdef CONFIG_PROC_SYSCTL

@@ -74,6 +75,9 @@ struct proc_maps_private {
 #ifdef CONFIG_MMU
 	struct vm_area_struct *tail_vma;
 #endif
+#ifdef CONFIG_NUMA
+	struct mempolicy *task_mempolicy;
+#endif
 };
  void proc_init_inodecache(void);

fs/proc/task_mmu.c +0 -0

index 14df880..624927d 100644

So, take referenceount under task_lock() before we start
+ * scanning and drop it when numa_maps reaches the end.
+ */

fs/proc/task_mmu.c +21 -0

+static void hold_task_mempolicy(struct proc_maps_private *priv)
+{
+	struct task_struct *task = priv->task;
+
+	task_lock(task);
+	priv->task_mempolicy = task->mempolicy;
+	mpol_get(priv->task_mempolicy);
+	task_unlock(task);
+}
+static void release_task_mempolicy(struct proc_maps_private *priv)
+{
+	mpol_put(priv->task_mempolicy);
+}
+#else
+static void hold_task_mempolicy(struct proc_maps_private *priv)
+{
+}
+static void release_task_mempolicy(struct proc_maps_private *priv)
+{
+}
+#endif
  static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct

*vma)
{

fs/proc/task_mmu.c +2 -1

 	if (vma && vma != priv->tail_vma) {
 		struct mm_struct *mm = vma->vm_mm;
+		release_task_mempolicy(priv);
 		up_read(&mm->mmap_sem);
 		mmput(mm);
 	}

@@ -132,7 +162,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
  	tail_vma = get_gate_vma(priv->task->mm);
 	priv->tail_vma = tail_vma;
-
+	hold_task_mempolicy(priv);
 	/* Start with last addr hint */
 	vma = find_vma(mm, last_addr);
 	if (last_addr && vma) {

Quoted 11 lines · Kamezawa Hiroyuki

(quoted diff) +1 -1

@@ -132,7 +162,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
   	tail_vma = get_gate_vma(priv->task->mm);
  	priv->tail_vma = tail_vma;
-
+	hold_task_mempolicy(priv);
  	/* Start with last addr hint */
  	vma = find_vma(mm, last_addr);
  	if (last_addr && vma) {

@@ -159,6 +189,7 @@ out:
  	if (vma)
  		return vma;

Review out: · line 194

Quoted 4 lines

(quoted diff) +0 -0

  +	release_task_mempolicy(priv);
  	/* End of vmas has been reached */
  	m->version = (tail_vma != NULL)? 0: -1UL;
  	up_read(&mm->mmap_sem);

Otherwise looks good, but please remove the two task_lock()'s in show_numa_map() that I added as part of this since you're replacing the need for locking.

From c5849c9034abeec3f26bf30dadccd393b0c5c25e Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Fri, 19 Oct 2012 17:00:55 +0900
Subject: [PATCH] hold task->mempolicy while numa_maps scans.

/proc/<pid>/numa_maps scans vma and show mempolicy under mmap_sem. It sometimes accesses task->mempolicy which can be freed without mmap_sem and numa_maps can show some garbage while scanning.

This patch tries to take reference count of task->mempolicy at reading numa_maps before calling get_vma_policy(). By this, task->mempolicy will not be freed until numa_maps reaches its end.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Looks good, but the patch is whitespace damaged so it doesn't apply. When that's fixed:

Acked-by David Rientjes <[email protected]>

Quoted 4 lines · David Rientjes

Looks good, but the patch is whitespace damaged so it doesn't apply. When that's fixed:

Acked-by David Rientjes <[email protected]>

Sorry, I hope this one is not broken...

...

fs/proc/internal.h +1 -0

@@ -12,6 +12,7 @@
  #include <linux/sched.h>
  #include <linux/proc_fs.h>
  struct  ctl_table_header;
+struct  mempolicy;
 
  extern struct proc_dir_entry proc_root;
  #ifdef CONFIG_PROC_SYSCTL

Quoted 4 lines

Looks good, but the patch is whitespace damaged so it doesn't apply. When that's fixed:

Acked-by David Rientjes <[email protected]>

Sorry, I hope this one is not broken...

Quoted 15 lines · David Rientjes

Quoted 13 lines · Kamezawa Hiroyuki

fs/proc/task_mmu.c +1 -1

@@ -132,7 +162,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
        tail_vma = get_gate_vma(priv->task->mm);
        priv->tail_vma = tail_vma;
-
+       hold_task_mempolicy(priv);
        /* Start with last addr hint */
        vma = find_vma(mm, last_addr);
        if (last_addr && vma) {

@@ -159,6 +189,7 @@ out:
        if (vma)
                return vma;
  +     release_task_mempolicy(priv);

Quoted 9 lines

fs/proc/task_mmu.c out: · line 209

Quoted 3 lines

fs/proc/task_mmu.c +0 -0

        /* End of vmas has been reached */
        m->version = (tail_vma != NULL)? 0: -1UL;
        up_read(&mm->mmap_sem);

Otherwise looks good, but please remove the two task_lock()'s in show_numa_map() that I added as part of this since you're replacing the need for locking.

Thank you for your review.
How about this ?

==
From c5849c9034abeec3f26bf30dadccd393b0c5c25e Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Fri, 19 Oct 2012 17:00:55 +0900
Subject: [PATCH] hold task->mempolicy while numa_maps scans.

/proc/<pid>/numa_maps scans vma and show mempolicy under mmap_sem. It sometimes accesses task->mempolicy which can be freed without mmap_sem and numa_maps can show some garbage while scanning.

This patch tries to take reference count of task->mempolicy at reading numa_maps before calling get_vma_policy(). By this, task->mempolicy will not be freed until numa_maps reaches its end.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

V2->v3
- updated comments to be more verbose.
- removed task_lock() in numa_maps code.
V1->V2
- access task->mempolicy only once and remember it. Becase kernel/exit.c can overwrite it.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Quoted 3 lines

Can't we have another way to fix ? like this ? too ugly ? Again, I'm sorry if I misunderstand the points.

Sorry this patch itself may be buggy. please don't test.. I missed that kernel/exit.c sets task->mempolicy to be NULL. fixed one here.

--
From 5581c71e68a7f50e52fd67cca00148911023f9f5 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Thu, 18 Oct 2012 13:50:29 +0900

Subject: [PATCH] hold task->mempolicy while numa_maps scans.

/proc/<pid>/numa_maps scans vma and show mempolicy under mmap_sem. It sometimes accesses task->mempolicy which can be freed without mmap_sem and numa_maps can show some garbage while scanning.

This patch tries to take reference count of task->mempolicy at reading numa_maps before calling get_vma_policy(). By this, task->mempolicy will not be freed until numa_maps reaches its end.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

V1->V2
- access task->mempolicy only once and remember it. Becase kernel/exit.c can overwrite it.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

fs/proc/task_mmu.c +5 -0

index 14df880..d92e868 100644
@@ -94,6 +94,11 @@ static void vma_stop(struct proc_maps_private *priv, struct
 vm_area_struct *vma)
 {
 	if (vma && vma != priv->tail_vma) {
 		struct mm_struct *mm = vma->vm_mm;
+#ifdef CONFIG_NUMA
+		task_lock(priv->task);
+		__mpol_put(priv->task->mempolicy);
+		task_unlock(priv->task);
+#endif
 		up_read(&mm->mmap_sem);
 		mmput(mm);

fs/proc/task_mmu.c +9 -0

 	}

@@ -130,6 +135,16 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 		return mm;
 	down_read(&mm->mmap_sem);
 +	/*
+	 * task->mempolicy can be freed even if mmap_sem is down (see
 kernel/exit.c)
+	 * We grab refcount for stable access.
+	 * repleacement of task->mmpolicy is guarded by mmap_sem.
+	 */
+#ifdef CONFIG_NUMA
+	task_lock(priv->task);
+	mpol_get(priv->task->mempolicy);
+	task_unlock(priv->task);
+#endif
 	tail_vma = get_gate_vma(priv->task->mm);
 	priv->tail_vma = tail_vma;
 @@ -161,6 +176,11 @@ out:

Quoted 13 lines · Dave Jones

BUG: sleeping function called from invalid context at kernel/mutex.c:269
in_atomic(): 1, irqs_disabled(): 0, pid: 8558, name: trinity-child2
3 locks on stack by trinity-child2/8558:
 #0: held:     (&p->lock){+.+.+.}, instance: ffff88010c9a00b0, at: [<ffffffff8120cd1f>] seq_lseek+0x3f/0x120
 #1: held:     (&mm->mmap_sem){++++++}, instance: ffff88013956f7c8, at: [<ffffffff81254437>] m_start+0xa7/0x190
 #2: held:     (&(&p->alloc_lock)->rlock){+.+...}, instance: ffff88011fc64f30, at: [<ffffffff81254f8f>] show_numa_map+0x14f/0x610
Pid: 8558, comm: trinity-child2 Not tainted 3.7.0-rc1+ #32
Call Trace:
 [<ffffffff810ae4ec>] __might_sleep+0x14c/0x200
 [<ffffffff816bdf4e>] mutex_lock_nested+0x2e/0x50
 [<ffffffff811c43a3>] mpol_shared_policy_lookup+0x33/0x90
 [<ffffffff8118d5c3>] shmem_get_policy+0x33/0x40
 [<ffffffff811c31fa>] get_vma_policy+0x3a/0x90

Quoted 9 lines

 [<ffffffff81254fa3>] show_numa_map+0x163/0x610
 [<ffffffff81255b10>] ? pid_maps_open+0x20/0x20
 [<ffffffff81255980>] ? pagemap_hugetlb_range+0xf0/0xf0
 [<ffffffff81255483>] show_pid_numa_map+0x13/0x20
 [<ffffffff8120c902>] traverse+0xf2/0x230
 [<ffffffff8120cd8b>] seq_lseek+0xab/0x120
 [<ffffffff811e6c0b>] sys_lseek+0x7b/0xb0
 [<ffffffff816ca088>] tracesys+0xe1/0xe6

Hmm, looks like we need to change the refcount semantics entirely. We'll need to make get_vma_policy() always take a reference and then drop it accordingly. This work sif get_vma_policy() can grab a reference while holding task_lock() for the task policy fallback case.

Comments on this approach?
---

I'm not sure about the status of the patch, but it doesn't apply on top of -next, and I still
see the warnings when fuzzing on -next.

Quoted 4 lines · Sasha Levin

I'm not sure about the status of the patch, but it doesn't apply on top of -next, and I still
see the warnings when fuzzing on -next.

This should be fixed by 9e7814404b77 ("hold task->mempolicy while numa_maps scans.") in 3.7-rc2, can you reproduce any issues reading /proc/pid/numa_maps on that kernel?

Quoted 3 lines

This should be fixed by 9e7814404b77 ("hold task->mempolicy while numa_maps scans.") in 3.7-rc2, can you reproduce any issues reading /proc/pid/numa_maps on that kernel?

I was actually referring to the warnings Dave Jones saw when fuzzing with trinity after the
original patch was applied.

I still see the following when fuzzing:

[  338.467156] BUG: sleeping function called from invalid context at
kernel/mutex.c:269
[  338.473719] in_atomic(): 1, irqs_disabled(): 0, pid: 6361, name: trinity-main
[  338.481199] 2 locks held by trinity-main/6361:
[  338.486629]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff810aa314>]
__do_page_fault+0x1e4/0x4f0
[  338.498783]  #1:  (&(&mm->page_table_lock)->rlock){+.+...}, at:
[<ffffffff8122f017>] handle_pte_fault+0x3f7/0x6a0
[  338.511409] Pid: 6361, comm: trinity-main Tainted: G        W
3.7.0-rc2-next-20121024-sasha-00001-gd95ef01-dirty #74
[  338.530318] Call Trace:
[  338.534088]  [<ffffffff8114e393>] __might_sleep+0x1c3/0x1e0
[  338.539358]  [<ffffffff83ae5209>] mutex_lock_nested+0x29/0x50
[  338.545253]  [<ffffffff8124fc3e>] mpol_shared_policy_lookup+0x2e/0x90
[  338.545258]  [<ffffffff81219ebe>] shmem_get_policy+0x2e/0x30
[  338.545264]  [<ffffffff8124e99a>] get_vma_policy+0x5a/0xa0
[  338.545267]  [<ffffffff8124fce1>] mpol_misplaced+0x41/0x1d0
[  338.545272]  [<ffffffff8122f085>] handle_pte_fault+0x465/0x6a0
[  338.545278]  [<ffffffff81131e04>] ? __rcu_read_unlock+0x44/0xb0
[  338.545282]  [<ffffffff81230baa>] handle_mm_fault+0x32a/0x360
[  338.545286]  [<ffffffff810aa5b0>] __do_page_fault+0x480/0x4f0

[  338.545293]  [<ffffffff8111a706>] ? del_timer+0x26/0x80
[  338.545298]  [<ffffffff811c7313>] ? rcu_cleanup_after_idle+0x23/0x170
[  338.545302]  [<ffffffff811ca9a4>] ? rcu_eqs_exit_common+0x64/0x3a0
[  338.545305]  [<ffffffff811c8c66>] ? rcu_eqs_enter_common+0x7c6/0x970
[  338.545309]  [<ffffffff811cafdc>] ? rcu_eqs_exit+0x9c/0xb0
[  338.545312]  [<ffffffff810aa666>] do_page_fault+0x26/0x40
[  338.545317]  [<ffffffff810a3a40>] do_async_page_fault+0x30/0xa0
[  338.545321]  [<ffffffff83ae9268>] async_page_fault+0x28/0x30

Quoted 31 lines · Sasha Levin

Quoted 3 lines

This should be fixed by 9e7814404b77 ("hold task->mempolicy while numa_maps scans.") in 3.7-rc2, can you reproduce any issues reading /proc/pid/numa_maps on that kernel?

I was actually referring to the warnings Dave Jones saw when fuzzing with trinity after the
original patch was applied.

I still see the following when fuzzing:

[  338.467156] BUG: sleeping function called from invalid context at
kernel/mutex.c:269
[  338.473719] in_atomic(): 1, irqs_disabled(): 0, pid: 6361, name: trinity-main
[  338.481199] 2 locks held by trinity-main/6361:
[  338.486629]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff810aa314>]
__do_page_fault+0x1e4/0x4f0
[  338.498783]  #1:  (&(&mm->page_table_lock)->rlock){+.+...}, at:
[<ffffffff8122f017>] handle_pte_fault+0x3f7/0x6a0
[  338.511409] Pid: 6361, comm: trinity-main Tainted: G        W
3.7.0-rc2-next-20121024-sasha-00001-gd95ef01-dirty #74
[  338.530318] Call Trace:
[  338.534088]  [<ffffffff8114e393>] __might_sleep+0x1c3/0x1e0
[  338.539358]  [<ffffffff83ae5209>] mutex_lock_nested+0x29/0x50
[  338.545253]  [<ffffffff8124fc3e>] mpol_shared_policy_lookup+0x2e/0x90
[  338.545258]  [<ffffffff81219ebe>] shmem_get_policy+0x2e/0x30
[  338.545264]  [<ffffffff8124e99a>] get_vma_policy+0x5a/0xa0
[  338.545267]  [<ffffffff8124fce1>] mpol_misplaced+0x41/0x1d0
[  338.545272]  [<ffffffff8122f085>] handle_pte_fault+0x465/0x6a0
[  338.545278]  [<ffffffff81131e04>] ? __rcu_read_unlock+0x44/0xb0
[  338.545282]  [<ffffffff81230baa>] handle_mm_fault+0x32a/0x360
[  338.545286]  [<ffffffff810aa5b0>] __do_page_fault+0x480/0x4f0

Quoted 9 lines

[  338.545293]  [<ffffffff8111a706>] ? del_timer+0x26/0x80
[  338.545298]  [<ffffffff811c7313>] ? rcu_cleanup_after_idle+0x23/0x170
[  338.545302]  [<ffffffff811ca9a4>] ? rcu_eqs_exit_common+0x64/0x3a0
[  338.545305]  [<ffffffff811c8c66>] ? rcu_eqs_enter_common+0x7c6/0x970
[  338.545309]  [<ffffffff811cafdc>] ? rcu_eqs_exit+0x9c/0xb0
[  338.545312]  [<ffffffff810aa666>] do_page_fault+0x26/0x40
[  338.545317]  [<ffffffff810a3a40>] do_async_page_fault+0x30/0xa0
[  338.545321]  [<ffffffff83ae9268>] async_page_fault+0x28/0x30

Ok, this looks the same but it's actually a different issue:
mpol_misplaced(), which now only exists in linux-next and not in 3.7-rc2, calls get_vma_policy() which may take the shared policy mutex. This happens while holding page_table_lock from do_huge_pmd_numa_page() but also from do_numa_page() while holding a spinlock on the ptl, which is coming from the sched/numa branch.

Is there anyway that we can avoid changing the shared policy mutex back into a spinlock (it was converted in b22d127a39dd ["mempolicy: fix a race in shared_policy_replace()"])?

Adding Peter, Rik, and Mel to the cc.

Hrm. I haven't noticed there is mpol_misplaced() in linux-next. Peter, I guess you commited it, right? If so, may I review your mempolicy changes? Now mempolicy has a lot of horrible buggy code and I hope to maintain carefully. Which tree should i see?

Ok, this looks the same but it's actually a different issue:
mpol_misplaced(), which now only exists in linux-next and not in 3.7-rc2, calls get_vma_policy() which may take the shared policy mutex. This happens while holding page_table_lock from do_huge_pmd_numa_page() but also from do_numa_page() while holding a spinlock on the ptl, which is coming from the sched/numa branch.

Is there anyway that we can avoid changing the shared policy mutex back into a spinlock (it was converted in b22d127a39dd ["mempolicy: fix a race in shared_policy_replace()"])?

Adding Peter, Rik, and Mel to the cc.

Quoted 7 lines · David Rientjes

Ok, this looks the same but it's actually a different issue:
mpol_misplaced(), which now only exists in linux-next and not in 3.7-rc2, calls get_vma_policy() which may take the shared policy mutex. This happens while holding page_table_lock from do_huge_pmd_numa_page() but also from do_numa_page() while holding a spinlock on the ptl, which is coming from the sched/numa branch.

Quoted 5 lines

Is there anyway that we can avoid changing the shared policy mutex back into a spinlock (it was converted in b22d127a39dd ["mempolicy: fix a race in shared_policy_replace()"])?

Adding Peter, Rik, and Mel to the cc.

Urgh, crud I totally missed that.

So the problem is that we need to compute if the current page is placed 'right' while holding pte_lock in order to avoid multiple pte_lock acquisitions on the 'fast' path.

I'll look into this in a bit, but one thing that comes to mind is having both a spnilock and a mutex and require holding both for modification while either one is sufficient for read.

That would allow sp_lookup() to use the spinlock, while insert and replace can hold both.

Not sure it will work for this, need to stare at this code a little more.

Quoted 24 lines · Peter Zijlstra

Quoted 7 lines · David Rientjes

Ok, this looks the same but it's actually a different issue:
mpol_misplaced(), which now only exists in linux-next and not in 3.7-rc2, calls get_vma_policy() which may take the shared policy mutex. This happens while holding page_table_lock from do_huge_pmd_numa_page() but also from do_numa_page() while holding a spinlock on the ptl, which is coming from the sched/numa branch.

Quoted 5 lines

Is there anyway that we can avoid changing the shared policy mutex back into a spinlock (it was converted in b22d127a39dd ["mempolicy: fix a race in shared_policy_replace()"])?

Adding Peter, Rik, and Mel to the cc.

Urgh, crud I totally missed that.

So the problem is that we need to compute if the current page is placed 'right' while holding pte_lock in order to avoid multiple pte_lock acquisitions on the 'fast' path.

I'll look into this in a bit, but one thing that comes to mind is having both a spnilock and a mutex and require holding both for modification while either one is sufficient for read.

Quoted 5 lines

That would allow sp_lookup() to use the spinlock, while insert and replace can hold both.

Not sure it will work for this, need to stare at this code a little more.

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

---

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

Quoted 7 lines · Peter Zijlstra

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

Looks reasonable, if annoyingly complex for something that shouldn't be important enough for this. Oh well.

However, please check me on this: the need for this is only for linux-next right now, correct? All the current users in my tree are ok with just the mutex, no?

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Peter Zijlstra

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

Looks reasonable, if annoyingly complex for something that shouldn't be important enough for this. Oh well.

I agree with that.. Its just that when doing numa placement one needs to respect the pre-existing placement constraints. I've not seen a way around this.

Quoted 3 lines

However, please check me on this: the need for this is only for linux-next right now, correct? All the current users in my tree are ok with just the mutex, no?

Yes, the need comes from the numa stuff and I'll stick this patch in there.

I completely missed Mel's patch turning it into a mutex, but I guess that's what -next is for :-).

Quoted 21 lines · Peter Zijlstra

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Peter Zijlstra

So I think the below should work, we hold the spinlock over both rb-tree modification as sp free, this makes mpol_shared_policy_lookup() which returns the policy with an incremented refcount work with just the spinlock.

Comments?

Looks reasonable, if annoyingly complex for something that shouldn't be important enough for this. Oh well.

I agree with that.. Its just that when doing numa placement one needs to respect the pre-existing placement constraints. I've not seen a way around this.

Quoted 3 lines

However, please check me on this: the need for this is only for linux-next right now, correct? All the current users in my tree are ok with just the mutex, no?

Quoted 5 lines

Yes, the need comes from the numa stuff and I'll stick this patch in there.

I completely missed Mel's patch turning it into a mutex, but I guess that's what -next is for :-).

So I've been fuzzing with it for the past couple of days and it's been looking fine with it. Can someone grab it into his tree please?

Thanks,
Sasha

Quoted 7 lines · KOSAKI Motohiro

Quoted 7 lines

Quoted 3 lines

Even though 80de7c3138ee9fd86a98696fd2cf7ad89b995d0a itself is simple. It bring to caller complex. That's not good and have no worth.

Before: the kernel panics, all workloads cease.
After: the file shows garbage, all workloads continue.

Quoted 4 lines

Quoted 2 lines

This is better, in my opinion, but at best it's only a judgment call and has no effect on anything.

Kernel panics help to find our serious mistake.

Kernel panics are not your little debugging tool to let users suffer

through for non-fatal issues.

Quoted 7 lines

Quoted 6 lines

I agree it would be better to respect the return value of mpol_to_str() since there are other possible error conditions other than a freed mempolicy, but let's not consider reverting 80de7c3138. It is obviously not a full solution to the problem, though, and we need to serialize with task_lock().

Quoted 1 line

Sorry no. I will have to revert it.

Feel free to revert anything you wish in your own tree, I couldn't care less. If you try to propose it upstream, Andrew will surely ask you to justify the BUG(), good luck on that.

I'll reply to this message with the fix that I think is best.