[PATCH] holepunch: fix mmap_sem i_mutex deadlock

sys_madvise has down_write of mmap_sem, then madvise_remove calls vmtruncate_range which takes i_mutex and i_alloc_sem: no, we can easily devise deadlocks from that ordering. madvise_remove drop mmap_sem while calling vmtruncate_range: luckily, since madvise_remove doesn't split or merge vmas, it's easy to handle this case with a NULL prev, without restructuring sys_madvise. (Though sad to retake mmap_sem when it's unlikely to be needed, and certainly down_read is sufficient for MADV_REMOVE, unlike the other madvices.) Signed-off-by: Hugh Dickins <hugh@veritas.com> Cc: Miklos Szeredi <mszeredi@suse.cz> Cc: Badari Pulavarty <pbadari@us.ibm.com> Cc: Nick Piggin <npiggin@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[PATCH] holepunch: fix mmap_sem i_mutex deadlock
sys_madvise has down_write of mmap_sem, then madvise_remove calls vmtruncate_range which takes i_mutex and i_alloc_sem: no, we can easily devise deadlocks from that ordering. madvise_remove drop mmap_sem while calling vmtruncate_range: luckily, since madvise_remove doesn't split or merge vmas, it's easy to handle this case with a NULL prev, without restructuring sys_madvise. (Though sad to retake mmap_sem when it's unlikely to be needed, and certainly down_read is sufficient for MADV_REMOVE, unlike the other madvices.) Signed-off-by: Hugh Dickins <hugh@veritas.com> Cc: Miklos Szeredi <mszeredi@suse.cz> Cc: Badari Pulavarty <pbadari@us.ibm.com> Cc: Nick Piggin <npiggin@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Hugh Dickins · Linus Torvalds
1 parent 16a100190d
Showing 1 changed file with 14 additions and 5 deletions Side-by-side Diff
mm/madvise.c
@@ -159,9 +159,10 @@
 				unsigned long start, unsigned long end)
 {
 	struct address_space *mapping;
-        loff_t offset, endoff;
+	loff_t offset, endoff;
+	int error;
  
-	*prev = vma;
+	*prev = NULL;	/* tell sys_madvise we drop mmap_sem */
  
 	if (vma->vm_flags & (VM_LOCKED|VM_NONLINEAR|VM_HUGETLB))
 		return -EINVAL;
@@ -180,7 +181,12 @@
 			+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
 	endoff = (loff_t)(end - vma->vm_start - 1)
 			+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
-	return  vmtruncate_range(mapping->host, offset, endoff);
+
+	/* vmtruncate_range needs to take i_mutex and i_alloc_sem */
+	up_write(&current->mm->mmap_sem);
+	error = vmtruncate_range(mapping->host, offset, endoff);
+	down_write(&current->mm->mmap_sem);
+	return error;
 }
  
 static long
  
@@ -315,12 +321,15 @@
 		if (error)
 			goto out;
 		start = tmp;
-		if (start < prev->vm_end)
+		if (prev && start < prev->vm_end)
 			start = prev->vm_end;
 		error = unmapped_error;
 		if (start >= end)
 			goto out;
-		vma = prev->vm_next;
+		if (prev)
+			vma = prev->vm_next;
+		else	/* madvise_remove dropped mmap_sem */
+			vma = find_vma(current->mm, start);
 	}
 out:
 	up_write(&current->mm->mmap_sem);
...	...	@@ -159,9 +159,10 @@
159	159	unsigned long start, unsigned long end)
160	160	{
161	161	struct address_space *mapping;
162		- loff_t offset, endoff;
	162	+ loff_t offset, endoff;
	163	+ int error;
163	164
164		- *prev = vma;
	165	+ prev = NULL; / tell sys_madvise we drop mmap_sem */
165	166
166	167	if (vma->vm_flags & (VM_LOCKED\|VM_NONLINEAR\|VM_HUGETLB))
167	168	return -EINVAL;
...	...	@@ -180,7 +181,12 @@
180	181	+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
181	182	endoff = (loff_t)(end - vma->vm_start - 1)
182	183	+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
183		- return vmtruncate_range(mapping->host, offset, endoff);
	184	+
	185	+ /* vmtruncate_range needs to take i_mutex and i_alloc_sem */
	186	+ up_write(&current->mm->mmap_sem);
	187	+ error = vmtruncate_range(mapping->host, offset, endoff);
	188	+ down_write(&current->mm->mmap_sem);
	189	+ return error;
184	190	}
185	191
186	192	static long
187	193
...	...	@@ -315,12 +321,15 @@
315	321	if (error)
316	322	goto out;
317	323	start = tmp;
318		- if (start < prev->vm_end)
	324	+ if (prev && start < prev->vm_end)
319	325	start = prev->vm_end;
320	326	error = unmapped_error;
321	327	if (start >= end)
322	328	goto out;
323		- vma = prev->vm_next;
	329	+ if (prev)
	330	+ vma = prev->vm_next;
	331	+ else /* madvise_remove dropped mmap_sem */
	332	+ vma = find_vma(current->mm, start);
324	333	}
325	334	out:
326	335	up_write(&current->mm->mmap_sem);