buffered write deadlock fix

From: Andrew Morton <[email protected]> and Nick Piggin <[email protected]>

The idea is to modify the core write() code so that it won't take a pagefault while holding a lock on the pagecache page. There are a number of different deadlocks possible if we try to do such a thing:

1. generic_buffered_write
2. lock_page
3. prepare_write
4. unlock_page+vmtruncate
5. copy_from_user
6. mmap_sem(r)
7. handle_mm_fault
8. lock_page (filemap_nopage)
9. commit_write
1. unlock_page

b. sys_munmap / sys_mlock / others
c. mmap_sem(w)
d. make_pages_present
e. get_user_pages
f. handle_mm_fault
g. lock_page (filemap_nopage)

2,8 - recursive deadlock if page is same
2,8;2,7 - ABBA deadlock is page is different
2,6;c,g - ABBA deadlock if page is same

- Instead of copy_from_user(), use inc_preempt_count() and copy_from_user_inatomic().

- If the copy_from_user_inatomic() hits a pagefault, it'll return a short copy.

- if the page was not uptodate, we cannot commit the write, because the uncopied bit could have uninitialised data. Commit zero length copy, which should do the right thing (ie. not set the page uptodate).

- if the page was uptodate, commit the copied portion so we make some progress.

- unlock_page()

- go back and try to fault the page in again, redo the lock_page, prepare_write, copy_from_user_inatomic(), etc.

- Now we have a non uptodate page, and we keep faulting on a 2nd or later iovec, this gives a deadlock, because fault_in_pages readable only faults in the first iovec. To fix this situation, if we fault on a !uptodate page, make the next iteration only attempt to copy a single iovec.

- This also showed up a number of buggy prepare_write / commit_write implementations that were setting the page uptodate in the prepare_write
side: bad! this allows uninitialised data to be read. Fix these.

int simple_commit_write(struct file *file, struct page *page,

fs/libfs.c +1 -7

-			unsigned offset, unsigned to)
+			unsigned from, unsigned to)
 {
-	struct inode *inode = page->mapping->host;
-	loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to;
-
-	/*
-	 * No need to use i_size_read() here, the i_size
-	 * cannot change under us because we hold the i_mutex.

fs/buffer.c +0 -3

@@ -2317,17 +2320,6 @@ int nobh_prepare_write(struct page *page
 
 	if (is_mapped_to_disk)
 		SetPageMappedToDisk(page);
-	SetPageUptodate(page);
-
-	/*

Quoted 4 lines · On Fri, 13 Oct 2006 18:44:52 +0200 (CEST) Nick Piggin <[email protected]> wrote:

- This also showed up a number of buggy prepare_write / commit_write implementations that were setting the page uptodate in the prepare_write
side: bad! this allows uninitialised data to be read. Fix these.

Well. It's non-buggy under the current protocol because the page remains locked throughout. This patch would make these ->prepare_write() implementations buggy.

mm/filemap.c generic_file_buffered_write(struct kiocb · line 1988

Quoted 3 lines

mm/filemap.c +3 -0

+#ifdef CONFIG_DEBUG_VM
+			fault_in_pages_readable(buf, bytes);
+#endif

I'll need to remember to take that out later on. Or reorder the patches, I guess.

Quoted 14 lines

int simple_commit_write(struct file *file, struct page *page,

fs/libfs.c +1 -11

-			unsigned offset, unsigned to)
+			unsigned from, unsigned to)
 {
-	struct inode *inode = page->mapping->host;
-	loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to;
-
-	/*
-	 * No need to use i_size_read() here, the i_size
-	 * cannot change under us because we hold the i_mutex.
-	 */
-	if (pos > inode->i_size)
-		i_size_write(inode, pos);
-	set_page_dirty(page);

fs/libfs.c int simple_readpage(struct file *file, s · line 350

Quoted 6 lines

fs/libfs.c +6 -0

+	if (to > from) {
+		struct inode *inode = page->mapping->host;
+		loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to;
+
+		if (to - from == PAGE_CACHE_SIZE)
+			SetPageUptodate(page);

This SetPageUptodate() can go away?

Quoted 17 lines

fs/buffer.c +0 -11

@@ -2317,17 +2320,6 @@ int nobh_prepare_write(struct page *page
 
 	if (is_mapped_to_disk)
 		SetPageMappedToDisk(page);
-	SetPageUptodate(page);
-
-	/*
-	 * Setting the page dirty here isn't necessary for the prepare_write
-	 * function - commit_write will do that.  But if/when this function is
-	 * used within the pagefault handler to ensure that all mmapped pages
-	 * have backing space in the filesystem, we will need to dirty the page
-	 * if its contents were altered.
-	 */
-	if (dirtied_it)
-		set_page_dirty(page);
 

Review int nobh_prepare_write(struct page *page · line 2324

Quoted 1 line

(quoted diff) +0 -0

 	return 0;

Local variable `dirtied_it' can go away now.

Or can it? We've modified the page's contents. If the copy_from_user gets a fault and we do a zero-length ->commit_write(), nobody ends up dirtying the page.

fs/buffer.c int nobh_truncate_page(struct address_sp · line 2442

Quoted 8 lines

fs/buffer.c +1 -0

@@ -2450,6 +2436,7 @@ int nobh_truncate_page(struct address_sp
 		memset(kaddr + offset, 0, PAGE_CACHE_SIZE - offset);
 		flush_dcache_page(page);
 		kunmap_atomic(kaddr, KM_USER0);
+		SetPageUptodate(page);
 		set_page_dirty(page);
 	}
 	unlock_page(page);

I've already forgotten why this was added. Comment, please ;)

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Fri, 13 Oct 2006 18:44:52 +0200 (CEST) Nick Piggin <[email protected]> wrote:

- This also showed up a number of buggy prepare_write / commit_write implementations that were setting the page uptodate in the prepare_write
side: bad! this allows uninitialised data to be read. Fix these.

Quoted 3 lines

Well. It's non-buggy under the current protocol because the page remains locked throughout. This patch would make these ->prepare_write() implementations buggy.

But if it becomes uptodate, then do_generic_mapping_read can read it without locking it (and so can filemap_nopage at present, although it looks like that's going to take the page lock soon).

From: Andrew Morton <[email protected]> and Nick Piggin <[email protected]>

The idea is to modify the core write() code so that it won't take a pagefault while holding a lock on the pagecache page. There are a number of different deadlocks possible if we try to do such a thing:

Quoted 13 lines · Andrew Morton

fs/buffer.c int nobh_truncate_page(struct address_sp · line 2442

Quoted 8 lines · On Fri, 13 Oct 2006 18:44:52 +0200 (CEST) Nick Piggin <[email protected]> wrote:

fs/buffer.c +1 -0

@@ -2450,6 +2436,7 @@ int nobh_truncate_page(struct address_sp
 		memset(kaddr + offset, 0, PAGE_CACHE_SIZE - offset);
 		flush_dcache_page(page);
 		kunmap_atomic(kaddr, KM_USER0);
+		SetPageUptodate(page);
 		set_page_dirty(page);
 	}
 	unlock_page(page);

I've already forgotten why this was added. Comment, please ;)

Well, nobh_prepare_write no longer sets it uptodate, so we need to if we're going to set_page_dirty. OTOH, why does truncate_page need to zero the pagecache anyway? I wonder if we couldn't delete this whole function? (not in this patchset!)

The idea is to modify the core write() code so that it won't take a pagefault while holding a lock on the pagecache page. There are a number of different deadlocks possible if we try to do such a thing:

1. generic_buffered_write
2. lock_page
3. prepare_write
4. unlock_page+vmtruncate
5. copy_from_user
6. mmap_sem(r)
7. handle_mm_fault
8. lock_page (filemap_nopage)
9. commit_write
1. unlock_page

b. sys_munmap / sys_mlock / others
c. mmap_sem(w)
d. make_pages_present
e. get_user_pages
f. handle_mm_fault
g. lock_page (filemap_nopage)

2,8 - recursive deadlock if page is same
2,8;2,7 - ABBA deadlock is page is different

2,6;c,g - ABBA deadlock if page is same

mm/filemap.c +9 -1

+
+		/*
+		 * Must not enter the pagefault handler here, because we hold
+		 * the page lock, so we might recursively deadlock on the same
+		 * lock, or get an ABBA deadlock against a different lock, or
+		 * against the mmap_sem (which nests outside the page lock).
+		 * So increment preempt count, and use _atomic usercopies.
+		 */
+		inc_preempt_count();
 		if (likely(nr_segs == 1))
-			copied = filemap_copy_from_user(page, offset,

Index: linux-2.6/mm/filemap.h
===================================================================

mm/filemap.h +41 -26

@@ -22,19 +22,19 @@ __filemap_copy_from_user_iovec_inatomic(
 
 /*
  * Copy as much as we can into the page and return the number of bytes which
- * were sucessfully copied.  If a fault is encountered then clear the page
- * out to (offset+bytes) and return the number of bytes which were copied.
+ * were sucessfully copied.  If a fault is encountered then return the number of
+ * bytes which were copied.
  *
- * NOTE: For this to work reliably we really want copy_from_user_inatomic_nocache
- * to *NOT* zero any tail of the buffer that it failed to copy.  If it does,
- * and if the following non-atomic copy succeeds, then there is a small window
- * where the target page contains neither the data before the write, nor the
- * data after the write (it contains zero).  A read at this time will see
- * data that is inconsistent with any ordering of the read and the write.
- * (This has been detected in practice).
+ * NOTE: For this to work reliably we really want
+ * copy_from_user_inatomic_nocache to *NOT* zero any tail of the buffer that it
+ * failed to copy.  If it does, and if the following non-atomic copy succeeds,
+ * then there is a small window where the target page contains neither the data
+ * before the write, nor the data after the write (it contains zero).  A read at
+ * this time will see data that is inconsistent with any ordering of the read
+ * and the write.  (This has been detected in practice).
  */
 static inline size_t
-filemap_copy_from_user(struct page *page, unsigned long offset,
+filemap_copy_from_user_atomic(struct page *page, unsigned long offset,
 			const char __user *buf, unsigned bytes)
 {
 	char *kaddr;

@@ -44,23 +44,32 @@ filemap_copy_from_user(struct page *page
 	left = __copy_from_user_inatomic_nocache(kaddr + offset, buf, bytes);
 	kunmap_atomic(kaddr, KM_USER0);
 
-	if (left != 0) {
-		/* Do it the slow way */
-		kaddr = kmap(page);
-		left = __copy_from_user_nocache(kaddr + offset, buf, bytes);
-		kunmap(page);
-	}
+	return bytes - left;
+}
+
+static inline size_t
+filemap_copy_from_user_nonatomic(struct page *page, unsigned long offset,
+			const char __user *buf, unsigned bytes)
+{
+	char *kaddr;
+	int left;
+
+	kaddr = kmap(page);
+	left = __copy_from_user_nocache(kaddr + offset, buf, bytes);
+	kunmap(page);
+
 	return bytes - left;
 }
 
 /*
- * This has the same sideeffects and return value as filemap_copy_from_user().
+ * This has the same sideeffects and return value as
+ * filemap_copy_from_user_atomic().
  * The difference is that on a fault we need to memset the remainder of the
  * page (out to offset+bytes), to emulate filemap_copy_from_user()'s
  * single-segment behaviour.
  */
 static inline size_t
-filemap_copy_from_user_iovec(struct page *page, unsigned long offset,
+filemap_copy_from_user_iovec_atomic(struct page *page, unsigned long offset,
 			const struct iovec *iov, size_t base, size_t bytes)
 {
 	char *kaddr;

@@ -70,14 +79,27 @@ filemap_copy_from_user_iovec(struct page
 	copied = __filemap_copy_from_user_iovec_inatomic(kaddr + offset, iov,
 							 base, bytes);
 	kunmap_atomic(kaddr, KM_USER0);
-	if (copied != bytes) {
-		kaddr = kmap(page);
-		copied = __filemap_copy_from_user_iovec_inatomic(kaddr + offset, iov,
-								 base, bytes);
-		if (bytes - copied)
-			memset(kaddr + offset + copied, 0, bytes - copied);
-		kunmap(page);
-	}
+	return copied;
+}
+
+/*
+ * This has the same sideeffects and return value as
+ * filemap_copy_from_user_nonatomic().
+ * The difference is that on a fault we need to memset the remainder of the
+ * page (out to offset+bytes), to emulate filemap_copy_from_user_nonatomic()'s
+ * single-segment behaviour.
+ */
+static inline size_t
+filemap_copy_from_user_iovec_nonatomic(struct page *page, unsigned long offset,
+			const struct iovec *iov, size_t base, size_t bytes)
+{

Quoted 22 lines · Andrew Morton

The idea is to modify the core write() code so that it won't take a pagefault while holding a lock on the pagecache page. There are a number of different deadlocks possible if we try to do such a thing:

1. generic_buffered_write
2. lock_page
3. prepare_write
4. unlock_page+vmtruncate
5. copy_from_user
6. mmap_sem(r)
7. handle_mm_fault
8. lock_page (filemap_nopage)
9. commit_write
1. unlock_page

b. sys_munmap / sys_mlock / others
c. mmap_sem(w)
d. make_pages_present
e. get_user_pages
f. handle_mm_fault
g. lock_page (filemap_nopage)

Quoted 2 lines

2,8 - recursive deadlock if page is same
2,8;2,7 - ABBA deadlock is page is different

2,8;2,8 I think you mean

Quoted 16 lines

mm/filemap.c +10 -3

+		/*
+		 * Must not enter the pagefault handler here, because we hold
+		 * the page lock, so we might recursively deadlock on the same
+		 * lock, or get an ABBA deadlock against a different lock, or
+		 * against the mmap_sem (which nests outside the page lock).
+		 * So increment preempt count, and use _atomic usercopies.
+		 */
+		inc_preempt_count();
 		if (likely(nr_segs == 1))
-			copied = filemap_copy_from_user(page, offset,
+			copied = filemap_copy_from_user_atomic(page, offset,
 							buf, bytes);
 		else
-			copied = filemap_copy_from_user_iovec(page, offset,
-						cur_iov, iov_offset, bytes);
+			copied = filemap_copy_from_user_iovec_atomic(page,

mm/filemap.c generic_file_buffered_write(struct kiocb · line 1945

Quoted 4 lines

mm/filemap.c +4 -0

+						offset, cur_iov, iov_offset,
+						bytes);
+		dec_preempt_count();
+

Why use raw {inc,dec}_preempt_count() and not preempt_{disable,enable}()? Is the compiler barrier not needed here? And do we really want to avoid the preempt_check_resched()?

Quoted 21 lines

Index: linux-2.6/mm/filemap.h
===================================================================

mm/filemap.h +15 -0

@@ -22,19 +22,19 @@ __filemap_copy_from_user_iovec_inatomic(
 
+/*
+ * This has the same sideeffects and return value as
+ * filemap_copy_from_user_nonatomic().
+ * The difference is that on a fault we need to memset the remainder of the
+ * page (out to offset+bytes), to emulate filemap_copy_from_user_nonatomic()'s
+ * single-segment behaviour.
+ */
+static inline size_t
+filemap_copy_from_user_iovec_nonatomic(struct page *page, unsigned long offset,
+			const struct iovec *iov, size_t base, size_t bytes)
+{
+	char *kaddr;
+	size_t copied;
+
+	kaddr = kmap(page);

mm/filemap.h filemap_copy_from_user_iovec(struct page · line 104

Quoted 6 lines

mm/filemap.h +3 -0

+	copied = __filemap_copy_from_user_iovec_inatomic(kaddr + offset, iov,
+							 base, bytes);
+	kunmap(page);
 	return copied;
 }

Why create the _nonatomic versions? There are no users.

Quoted 16 lines

Quoted 16 lines

mm/filemap.c +10 -3

+		/*
+		 * Must not enter the pagefault handler here, because we hold
+		 * the page lock, so we might recursively deadlock on the same
+		 * lock, or get an ABBA deadlock against a different lock, or
+		 * against the mmap_sem (which nests outside the page lock).
+		 * So increment preempt count, and use _atomic usercopies.
+		 */
+		inc_preempt_count();
 		if (likely(nr_segs == 1))
-			copied = filemap_copy_from_user(page, offset,
+			copied = filemap_copy_from_user_atomic(page, offset,
 							buf, bytes);
 		else
-			copied = filemap_copy_from_user_iovec(page, offset,
-						cur_iov, iov_offset, bytes);
+			copied = filemap_copy_from_user_iovec_atomic(page,

Quoted 8 lines

mm/filemap.c generic_file_buffered_write(struct kiocb · line 1945

Quoted 4 lines

mm/filemap.c +4 -0

+						offset, cur_iov, iov_offset,
+						bytes);
+		dec_preempt_count();
+

Why use raw {inc,dec}_preempt_count() and not preempt_{disable,enable}()? Is the compiler barrier not needed here? And do we really want to avoid the preempt_check_resched()?

Counter to intuition, we actually don't mind being preempted here, but we do mind entering the (core) pagefault handler. Incrementing the preempt count causes the arch specific handler to bail out early before it takes any locks.

Clear as mud? Wrapping it in a better name might be an improvement? Or wrapping it into the copy*user_atomic functions themselves (which is AFAIK the only place we use it).

Quoted 11 lines

Quoted 5 lines

Why use raw {inc,dec}_preempt_count() and not preempt_{disable,enable}()? Is the compiler barrier not needed here? And do we really want to avoid the preempt_check_resched()?

Counter to intuition, we actually don't mind being preempted here, but we do mind entering the (core) pagefault handler. Incrementing the preempt count causes the arch specific handler to bail out early before it takes any locks.

Quoted 3 lines

Clear as mud? Wrapping it in a better name might be an improvement? Or wrapping it into the copy*user_atomic functions themselves (which is AFAIK the only place we use it).

Right, but since you do inc the preempt_count you do disable preemption, might as well check TIF_NEED_RESCHED when enabling preemption again.

Quoted 11 lines · Peter Zijlstra

Quoted 11 lines

Quoted 5 lines

Why use raw {inc,dec}_preempt_count() and not preempt_{disable,enable}()? Is the compiler barrier not needed here? And do we really want to avoid the preempt_check_resched()?

Counter to intuition, we actually don't mind being preempted here, but we do mind entering the (core) pagefault handler. Incrementing the preempt count causes the arch specific handler to bail out early before it takes any locks.

Quoted 6 lines

Quoted 3 lines

Clear as mud? Wrapping it in a better name might be an improvement? Or wrapping it into the copy*user_atomic functions themselves (which is AFAIK the only place we use it).

Right, but since you do inc the preempt_count you do disable preemption, might as well check TIF_NEED_RESCHED when enabling preemption again.

Yeah, you are right about that. Unfortunately there isn't a good way to do this at the moment... well we could disable preempt around the section, but that would be silly for a PREEMPT kernel.

And we should really decouple it from preempt entirely, in case we ever want to check for it some other way in the pagefault handler.

Quoted 3 lines

And we should really decouple it from preempt entirely, in case we ever want to check for it some other way in the pagefault handler.

How about we make sure all kmap_atomic implementations behave properly and make in_atomic true.

Quoted 4 lines · Peter Zijlstra

Quoted 3 lines

And we should really decouple it from preempt entirely, in case we ever want to check for it some other way in the pagefault handler.

Quoted 2 lines

How about we make sure all kmap_atomic implementations behave properly and make in_atomic true.

Hmm, but you may not be doing a copy*user within the kmap. And you may want an atomic copy*user not within a kmap (maybe).

I think it really would be more logical to do it in a wrapper function pagefault_disable() pagefault_enable()? ;)

Quoted 5 lines · Nick Piggin

Hmm, but you may not be doing a copy*user within the kmap. And you may want an atomic copy*user not within a kmap (maybe).

I think it really would be more logical to do it in a wrapper function pagefault_disable() pagefault_enable()? ;)

I did that one first, but then noticed that most non trivial kmap_atomic implementations already did the inc_preempt_count()/dec_preempt_count() thing (except frv which did preempt_disable()/preempt_enable() ?)

Anyway, here goes:

Signed-off-by Peter Zijlstra <[email protected]>

Index: linux-2.6/mm/filemap.h
===================================================================

mm/filemap.h +4 -0

@@ -21,6 +21,22 @@ __filemap_copy_from_user_iovec_inatomic(
 					size_t bytes);
 
 /*
+ * By increasing the preempt_count we make sure the arch preempt
+ * handler bails out early, before taking any locks, so that the copy
+ * operation gets terminated early.
+ */

(trimming cc list)

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Nick Piggin

Hmm, but you may not be doing a copy*user within the kmap. And you may want an atomic copy*user not within a kmap (maybe).

I think it really would be more logical to do it in a wrapper function pagefault_disable() pagefault_enable()? ;)

Quoted 7 lines

I did that one first, but then noticed that most non trivial kmap_atomic implementations already did the inc_preempt_count()/dec_preempt_count() thing (except frv which did preempt_disable()/preempt_enable() ?)

Anyway, here goes:

Signed-off-by Peter Zijlstra <[email protected]>

I think this is a good approach. The missed preempt checks could easily have been causing scheduling delays because the usercopy can take up a lot of kernel time.

I don't know that the function should go in filemap.h... uaccess.h seems more appropriate, and had thought the pagefault_disable() be calle directly from within the copy_*_user_inatomic functions themselves, not the filemap helper.

Also, the rest of the kernel tree (mainly uaccess and futexes) should be converted ;)

Quoted 7 lines

Index: linux-2.6/mm/filemap.h
===================================================================

mm/filemap.h +0 -0

@@ -21,6 +21,22 @@ __filemap_copy_from_user_iovec_inatomic(
 					size_t bytes);
 

mm/filemap.h __filemap_copy_from_user_iovec_inatomic( · line 30

Quoted 8 lines

mm/filemap.h +7 -0

 /*
+ * By increasing the preempt_count we make sure the arch preempt
+ * handler bails out early, before taking any locks, so that the copy
+ * operation gets terminated early.
+ */
+pagefault_static inline void disable(void)
+{
+	inc_preempt_count();

mm/filemap.h __filemap_copy_from_user_iovec_inatomic( · line 37

Quoted 7 lines

mm/filemap.h +7 -0

+}
+
+pagefault_static inline void enable(void)
+{
+	dec_preempt_count();
+	preempt_check_resched();
+}

Interesting prototype ;)

Quoted 2 lines · Nick Piggin

Also, the rest of the kernel tree (mainly uaccess and futexes) should be converted ;)

Yeah, lotsa places to touch.

Quoted 7 lines

Quoted 7 lines

Index: linux-2.6/mm/filemap.h
===================================================================

mm/filemap.h +0 -0

@@ -21,6 +21,22 @@ __filemap_copy_from_user_iovec_inatomic(
 					size_t bytes);
 

Quoted 8 lines

mm/filemap.h __filemap_copy_from_user_iovec_inatomic( · line 30

Quoted 8 lines

mm/filemap.h +7 -0

 /*
+ * By increasing the preempt_count we make sure the arch preempt
+ * handler bails out early, before taking any locks, so that the copy
+ * operation gets terminated early.
+ */
+pagefault_static inline void disable(void)
+{
+	inc_preempt_count();

I think we also need a barrier(); here. We need to make sure the preempt count is written to memory before we hit the fault handler.

Quoted 5 lines

mm/filemap.h __filemap_copy_from_user_iovec_inatomic( · line 37

Quoted 5 lines

mm/filemap.h +5 -0

+pagefault_static inline void enable(void)
+{
+	dec_preempt_count();
+	preempt_check_resched();
+}