Skip to contentSkip to conversation

fix put_user under mmap_sem in sys_get_mempolicy()

PATCH In review Oleg Nesterov 2 participants 4 messages Jan 21, 2005

Subsystems (2)
Memory management MM NUMA placement
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

PATCH 1/1 fix put_user under mmap_sem in sys_get_mempolicy()

Hello.

sys_get_mempolicy() accesses user memory with mmap_sem held. If I understand correctly, this can cause deadlock:

sys_get_mempolicy: Another thread, same mm:

down_read(mmap_sem);
				down_write(mmap_sem);

put_user();
do_page_fault:
down_read(mmap_sem);

Compile tested only, I have no NUMA machine.

Signed-off-by Oleg Nesterov <[email protected]>
mm/mempolicy.c renamed from mm/mempolicy.c~ +30 -24
@@ -482,26 +482,38 @@ asmlinkage long sys_get_mempolicy(int __
 				  unsigned long maxnode,
 				  unsigned long addr, unsigned long flags)
 {
-	int err, pval;
-	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma = NULL;
+	int err, pval = 0; /* make compiler happy */
 	struct mempolicy *pol = current->mempolicy;
 
 	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
 		return -EINVAL;
 	if (nmask != NULL && maxnode < MAX_NUMNODES)
 		return -EINVAL;
+
 	if (flags & MPOL_F_ADDR) {
+		struct mm_struct *mm = current->mm;
+		struct vm_area_struct *vma;
+
+		err = 0;
 		down_read(&mm->mmap_sem);
 		vma = find_vma_intersection(mm, addr, addr+1);
-		if (!vma) {
-			up_read(&mm->mmap_sem);
-			return -EFAULT;
+		if (!vma)
+			err = -EFAULT;
+		else {
+			if (vma->vm_ops && vma->vm_ops->get_policy)
+				pol = vma->vm_ops->get_policy(vma, addr);
+			else
+				pol = vma->vm_policy;
+
+			if (flags & MPOL_F_NODE) {
+				pval = lookup_node(mm, addr);
+				if (pval < 0)
+					err = pval;
+			}
 		}
-		if (vma->vm_ops && vma->vm_ops->get_policy)
-			pol = vma->vm_ops->get_policy(vma, addr);
-		else
-			pol = vma->vm_policy;
+		up_read(&mm->mmap_sem);
+		if (err)
+			goto out;
 	} else if (addr)
 		return -EINVAL;
 

@@ -509,17 +521,14 @@ asmlinkage long sys_get_mempolicy(int __
 		pol = &default_policy;
 
 	if (flags & MPOL_F_NODE) {
-		if (flags & MPOL_F_ADDR) {
-			err = lookup_node(mm, addr);
-			if (err < 0)
+		if (!(flags & MPOL_F_ADDR)) {
+			if (pol == current->mempolicy &&
+					pol->policy == MPOL_INTERLEAVE) {
+				pval = current->il_next;
+			} else {
+				err = -EINVAL;
 				goto out;
-			pval = err;
-		} else if (pol == current->mempolicy &&
-				pol->policy == MPOL_INTERLEAVE) {
-			pval = current->il_next;
-		} else {
-			err = -EINVAL;
-			goto out;
+			}
 		}
 	} else
 		pval = pol->policy;

@@ -534,10 +543,7 @@ asmlinkage long sys_get_mempolicy(int __
 		get_zonemask(pol, nodes);
 		err = copy_nodes_to_user(nmask, maxnode, nodes, sizeof(nodes));
 	}
-
- out:
-	if (vma)
-		up_read(&current->mm->mmap_sem);
+out:
 	return err;
 }
#1 lore raw
Quoted 5 lines · Oleg Nesterov

Hello.

sys_get_mempolicy() accesses user memory with mmap_sem held. If I understand correctly, this can cause deadlock:

Quoted 7 lines

sys_get_mempolicy: Another thread, same mm:

down_read(mmap_sem);
				down_write(mmap_sem);

put_user();
do_page_fault:
down_read(mmap_sem);

Hrm. Normal Linux policy seems to ignore this potential and rare deadlock and using nesting safely (e.g. it's been known for a long time for the tasklist rw spinlock, but nobody really cares and it doesn't seem to be hit by users). Do you really think it is likely to happen?

Quoted 2 lines

Compile tested only, I have no NUMA machine.

It's hard to figure out what your patch actually does because of all the gratious white space changes.

I suppose this simpler patch has the same effect (also untested).

mm/mempolicy.c renamed from mm/mempolicy.c-o +6 -3
diff -u linux-2.6.11-rc1-bk4/mm/mempolicy.c-o linux-2.6.11-rc1-bk4/mm/mempolicy.c
@@ -485,7 +485,7 @@
 	int err, pval;
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = NULL;
-	struct mempolicy *pol = current->mempolicy;
+	struct mempolicy *pol = current->mempolicy, *pol2 = NULL;
 
 	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
 		return -EINVAL;

@@ -502,6 +502,10 @@
 			pol = vma->vm_ops->get_policy(vma, addr);
 		else
 			pol = vma->vm_policy;
+		pol2 = mpol_copy(pol);
+		up_read(&mm->mmap_sem);
+		if (IS_ERR(pol2)) 
+			return PTR_ERR(pol2);
 	} else if (addr)
 		return -EINVAL;
 

@@ -536,8 +540,7 @@
 	}
 
  out:
-	if (vma)
-		up_read(&current->mm->mmap_sem);
+	mpol_free(pol2);
 	return err;
 }
author
#2 lore raw
Quoted 6 lines · Andi Kleen

I suppose this simpler patch has the same effect (also untested).

	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
		return -EINVAL;

mm/mempolicy.c line 509

Quoted 8 lines
mm/mempolicy.c renamed from mm/mempolicy.c-o +4 -0
@@ -502,6 +502,10 @@
 			pol = vma->vm_ops->get_policy(vma, addr);
 		else
 			pol = vma->vm_policy;
+		pol2 = mpol_copy(pol);
+		up_read(&mm->mmap_sem);
+		if (IS_ERR(pol2)) 
+			return PTR_ERR(pol2);

I don't think so. With MPOL_F_ADDR|MPOL_F_NODE sys_get_mempolicy calls lookup_node()->get_user_pages() few lines below, so we can't up_read(&mm->mmap_sem) here.

Quoted 2 lines

It's hard to figure out what your patch actually does because of all the gratious white space changes.

For your convenience here is the code with the patch applied.

	if (flags & MPOL_F_ADDR) {
		struct mm_struct *mm = current->mm;
		struct vm_area_struct *vma;

		err = 0;
		down_read(&mm->mmap_sem);
		vma = find_vma_intersection(mm, addr, addr+1);
		if (!vma)
			err = -EFAULT;
		else {
			if (vma->vm_ops && vma->vm_ops->get_policy)
				pol = vma->vm_ops->get_policy(vma, addr);
			else
				pol = vma->vm_policy;

			if (flags & MPOL_F_NODE) {
				pval = lookup_node(mm, addr);
				if (pval < 0)
					err = pval;
			}
		}
		up_read(&mm->mmap_sem);
		if (err)
			goto out;
	} else if (addr)
		return -EINVAL;

	if (!pol)
		pol = &default_policy;

	if (flags & MPOL_F_NODE) {
		if (!(flags & MPOL_F_ADDR)) {
			if (pol == current->mempolicy &&
					pol->policy == MPOL_INTERLEAVE) {
				pval = current->il_next;
			} else {
				err = -EINVAL;
				goto out;
			}
		}
	} else
		pval = pol->policy;
#3 lore raw
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines · Andi Kleen

I suppose this simpler patch has the same effect (also untested).

Quoted 15 lines

mm/mempolicy.c line 509

Quoted 11 lines
	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
		return -EINVAL;
mm/mempolicy.c renamed from mm/mempolicy.c-o +4 -0
@@ -502,6 +502,10 @@
 			pol = vma->vm_ops->get_policy(vma, addr);
 		else
 			pol = vma->vm_policy;
+		pol2 = mpol_copy(pol);
+		up_read(&mm->mmap_sem);
+		if (IS_ERR(pol2)) 
+			return PTR_ERR(pol2);

I don't think so. With MPOL_F_ADDR|MPOL_F_NODE sys_get_mempolicy calls lookup_node()->get_user_pages() few lines below, so we can't up_read(&mm->mmap_sem) here.

True.

Quoted 5 lines
Quoted 3 lines

It's hard to figure out what your patch actually does because of all the gratious white space changes.

For your convenience here is the code with the patch applied.

Looks reasonable.

-Andi

P.S.: Again if you really care about these class of deadlocks take a look at tasklist_lock.