Attachment 306018 Details for Bug 439548 – the mmap_sem deadlock prevention patch. Path 2/2

[patch] the mmap_sem deadlock prevention patch. Path 2/2

mmap-journal-deadlock.patch (text/plain), 6.61 KB, created by Josef Bacik on 2008-05-19 21:35:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Bacik

Created: 2008-05-19 21:35:11 UTC

Size: 6.61 KB

patch

obsolete

>Index: linux-rhel4-internal/include/linux/pagemap.h
>===================================================================
>--- linux-rhel4-internal.orig/include/linux/pagemap.h
>+++ linux-rhel4-internal/include/linux/pagemap.h
>@@ -201,6 +201,9 @@ static inline int fault_in_pages_writeab
> {
> 	int ret;
> 
>+	if (unlikely(size == 0))
>+		return 0;
>+
> 	/*
> 	 * Writing zeroes into userspace here is OK, because we know that if
> 	 * the zero gets there, we'll be overwriting it.
>@@ -220,19 +223,23 @@ static inline int fault_in_pages_writeab
> 	return ret;
> }
> 
>-static inline void fault_in_pages_readable(const char __user *uaddr, int size)
>+static inline int fault_in_pages_readable(const char __user *uaddr, int size)
> {
> 	volatile char c;
> 	int ret;
> 
>+	if (unlikely(size == 0))
>+		return 0;
>+
> 	ret = __get_user(c, uaddr);
> 	if (ret == 0) {
> 		const char __user *end = uaddr + size - 1;
> 
> 		if (((unsigned long)uaddr & PAGE_MASK) !=
> 				((unsigned long)end & PAGE_MASK))
>-		 	__get_user(c, end);
>+			ret = __get_user(c, end);
> 	}
>+	return ret;
> }
> 
> #endif /* _LINUX_PAGEMAP_H */
>Index: linux-rhel4-internal/mm/filemap.c
>===================================================================
>--- linux-rhel4-internal.orig/mm/filemap.c
>+++ linux-rhel4-internal/mm/filemap.c
>@@ -27,6 +27,7 @@
> #include <linux/pagevec.h>
> #include <linux/blkdev.h>
> #include <linux/security.h>
>+#include <linux/hardirq.h>
> /*
>  * This is needed for the following functions:
>  *  - try_to_release_page
>@@ -1979,7 +1980,6 @@ generic_file_buffered_write(struct kiocb
> 	long		status = 0;
> 	struct page	*page;
> 	struct page	*cached_page = NULL;
>-	size_t		bytes;
> 	struct pagevec	lru_pvec;
> 	const struct iovec *cur_iov = iov; /* current iovec */
> 	size_t		iov_base = 0;	   /* offset in the current iovec */
>@@ -1998,9 +1998,11 @@ generic_file_buffered_write(struct kiocb
> 	}
> 
> 	do {
>+		struct page *src_page;
> 		unsigned long index;
> 		unsigned long offset;
>-		unsigned long maxlen;
>+		unsigned long seglen;	/* Bytes remaining in current iovec */
>+		unsigned long bytes;	/* Bytes to write to page */
> 		size_t copied;
> 
> 		offset = (pos & (PAGE_CACHE_SIZE -1)); /* Within page */
>@@ -2010,21 +2012,82 @@ generic_file_buffered_write(struct kiocb
> 			bytes = count;
> 
> 		/*
>+		 * a non-NULL src_page indicates that we're doing the
>+		 * copy via get_user_pages and kmap
>+		 */
>+		src_page = NULL;
>+
>+		seglen = cur_iov->iov_len - iov_base;
>+		if (seglen > bytes)
>+			seglen = bytes;
>+
>+		/*
> 		 * Bring in the user page that we will copy from _first_.
>-		 * Otherwise there's a nasty deadlock on copying from the
>+		 * Othersize there's a nasty deadlock on copying from the
> 		 * same page as we're writing to, without it being marked
> 		 * up-to-date.
>+		 *
>+		 * Not only is this an optimisation, but it is also required
>+		 * to check that the address is actually valid, when atomic
>+		 * usercopies are used, below.
> 		 */
>-		maxlen = cur_iov->iov_len - iov_base;
>-		if (maxlen > bytes)
>-			maxlen = bytes;
>-		fault_in_pages_readable(buf, maxlen);
>+		if (unlikely(fault_in_pages_readable(buf, seglen))) {
>+			status = -EFAULT;
>+			break;
>+		}
>+
> 		page = __grab_cache_page(mapping,index,&cached_page,&lru_pvec);
> 		if (!page) {
> 			status = -ENOMEM;
> 			break;
> 		}
> 
>+		/*
>+		 * non-uptodate pages cannot cope with short copies, and we
>+		 * cannot take a pagefault with the destination page locked.
>+		 * So pin the source page to copy it.
>+		 */
>+		if (!PageUptodate(page)) {
>+			unlock_page(page);
>+
>+			src_page = alloc_page(GFP_KERNEL);
>+			if (!src_page) {
>+				page_cache_release(page);
>+				status = -ENOMEM;
>+				break;
>+			}
>+
>+			/*
>+			 * Cannot get_user_pages with a page locked for the
>+			 * same reason as we can't take a page fault with a
>+			 * page locked (as explained below).
>+			 */
>+			copied = filemap_copy_from_user_iovec(src_page, offset,
>+						cur_iov, iov_base, bytes);
>+			if (unlikely(copied == 0)) {
>+				status = -EFAULT;
>+				page_cache_release(page);
>+				page_cache_release(src_page);
>+				break;
>+			}
>+			bytes = copied;
>+
>+			lock_page(page);
>+			/*
>+			 * Can't handle the page going uptodate here, because
>+			 * that means we would use non-atomic usercopies, which
>+			 * zero out the tail of the page, which can cause
>+			 * zeroes to become transiently visible. We could just
>+			 * use a non-zeroing copy, but the APIs aren't too
>+			 * consistent.
>+			 */
>+			if (unlikely(!page->mapping || PageUptodate(page))) {
>+				unlock_page(page);
>+				page_cache_release(page);
>+				page_cache_release(src_page);
>+				continue;
>+			}
>+		}
> 		status = a_ops->prepare_write(file, page, offset, offset+bytes);
> 		if (unlikely(status)) {
> 			loff_t isize = i_size_read(inode);
>@@ -2038,12 +2101,46 @@ generic_file_buffered_write(struct kiocb
> 				vmtruncate(inode, isize);
> 			break;
> 		}
>-		if (likely(nr_segs == 1))
>-			copied = filemap_copy_from_user(page, offset,
>-							buf, bytes);
>-		else
>-			copied = filemap_copy_from_user_iovec(page, offset,
>-						cur_iov, iov_base, bytes);
>+
>+		if (!src_page) {
>+			char *kaddr;
>+			/*
>+			 * 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 agains the mmap_sem
>+			 * (which nests outside the page lock).  So increment
>+			 * preempt count, and use _atomic usercopies.
>+			 *
>+			 * The page is uptodate so we are OK to encounter a
>+			 * short copy: if an unmodified parts of the page are
>+			 * marked dirty and written out to disk, it doesn't
>+			 * really matter
>+			 */
>+			pagefault_disable();
>+			kaddr = kmap_atomic(page, KM_USER0);
>+			if (likely(nr_segs == 1)) {
>+				int left;
>+				char __user *buf = cur_iov->iov_base+iov_base;
>+				left = __copy_from_user_inatomic(kaddr+offset,
>+						buf, bytes);
>+				copied = bytes - left;
>+			} else {
>+				copied = __filemap_copy_from_user_iovec(kaddr +
>+						offset, cur_iov, iov_base, bytes);
>+			}
>+			kunmap_atomic(kaddr, KM_USER0);
>+
>+			pagefault_enable();
>+		} else {
>+			void *src, *dst;
>+			src = kmap_atomic(src_page, KM_USER0);
>+			dst = kmap_atomic(page, KM_USER1);
>+			memcpy(dst + offset, src + offset, bytes);
>+			kunmap_atomic(dst, KM_USER1);
>+			kunmap_atomic(src, KM_USER0);
>+			copied = bytes;
>+		}
> 		flush_dcache_page(page);
> 		status = a_ops->commit_write(file, page, offset, offset+bytes);
> 		if (likely(copied > 0)) {
>@@ -2065,12 +2162,12 @@ generic_file_buffered_write(struct kiocb
> 				}
> 			}
> 		}
>-		if (unlikely(copied != bytes))
>-			if (status >= 0)
>-				status = -EFAULT;
> 		unlock_page(page);
> 		mark_page_accessed(page);
> 		page_cache_release(page);
>+		if (src_page)
>+			page_cache_release(src_page);
>+
> 		if (status < 0)
> 			break;
> 		balance_dirty_pages_ratelimited(mapping);

Actions: View | Diff

Attachments on bug 439548: 306017 | 306018 | 308564 | 310833 | 328208 | 328578 | 328771 | 328891