[PATCH] splice: fix bugs in pipe_to_file()

Found by Oleg Nesterov <oleg@tv-sign.ru>, fixed by me.

- Only allow full pages to go to the page cache.
- Check page != buf->page instead of using PIPE_BUF_FLAG_STOLEN.
- Remember to clear 'stolen' if add_to_page_cache() fails.

And as a cleanup on that:

- Make the bottom fall-through logic a little less convoluted. Also make
  the steal path hold an extra reference to the page, so we don't have
  to differentiate between stolen and non-stolen at the end.

Signed-off-by: Jens Axboe <axboe@suse.de>

diff --git a/fs/pipe.c b/fs/pipe.c
index 5a369273c51b815465075f6c13ad5e65e4cbc031..888f265011bf46b539510d8f9ffa13a5eb844c7f 100644 (file)
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -99,8 +99,6 @@ static void anon_pipe_buf_release(struct pipe_inode_info *pipe,
 {
        struct page *page = buf->page;
 
-       buf->flags &= ~PIPE_BUF_FLAG_STOLEN;
-
        /*
         * If nobody else uses this page, and we don't already have a
         * temporary page, let's keep track of it as a one-deep
@@ -130,7 +128,6 @@ static int anon_pipe_buf_steal(struct pipe_inode_info *pipe,
        struct page *page = buf->page;
 
        if (page_count(page) == 1) {
-               buf->flags |= PIPE_BUF_FLAG_STOLEN;
                lock_page(page);
                return 0;
        }

diff --git a/fs/splice.c b/fs/splice.c
index 9df28d30efa0ede1eb5f149fd342d3f1c5b6cd8e..1633778f36528a9179de90222e7bf81a544d6e77 100644 (file)
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -78,7 +78,7 @@ static int page_cache_pipe_buf_steal(struct pipe_inode_info *info,
                return 1;
        }
 
-       buf->flags |= PIPE_BUF_FLAG_STOLEN | PIPE_BUF_FLAG_LRU;
+       buf->flags |= PIPE_BUF_FLAG_LRU;
        return 0;
 }
 
@@ -87,7 +87,7 @@ static void page_cache_pipe_buf_release(struct pipe_inode_info *info,
 {
        page_cache_release(buf->page);
        buf->page = NULL;
-       buf->flags &= ~(PIPE_BUF_FLAG_STOLEN | PIPE_BUF_FLAG_LRU);
+       buf->flags &= ~PIPE_BUF_FLAG_LRU;
 }
 
 static void *page_cache_pipe_buf_map(struct file *file,
@@ -587,9 +587,10 @@ static int pipe_to_file(struct pipe_inode_info *info, struct pipe_buffer *buf,
                this_len = PAGE_CACHE_SIZE - offset;
 
        /*
-        * Reuse buf page, if SPLICE_F_MOVE is set.
+        * Reuse buf page, if SPLICE_F_MOVE is set and we are doing a full
+        * page.
         */
-       if (sd->flags & SPLICE_F_MOVE) {
+       if ((sd->flags & SPLICE_F_MOVE) && this_len == PAGE_CACHE_SIZE) {
                /*
                 * If steal succeeds, buf->page is now pruned from the vm
                 * side (LRU and page cache) and we can reuse it. The page
@@ -604,6 +605,8 @@ static int pipe_to_file(struct pipe_inode_info *info, struct pipe_buffer *buf,
                        goto find_page;
                }
 
+               page_cache_get(page);
+
                if (!(buf->flags & PIPE_BUF_FLAG_LRU))
                        lru_cache_add(page);
        } else {
@@ -662,7 +665,7 @@ find_page:
        } else if (ret)
                goto out;
 
-       if (!(buf->flags & PIPE_BUF_FLAG_STOLEN)) {
+       if (buf->page != page) {
                char *dst = kmap_atomic(page, KM_USER0);
 
                memcpy(dst + offset, src + buf->offset, this_len);
@@ -671,22 +674,20 @@ find_page:
        }
 
        ret = mapping->a_ops->commit_write(file, page, offset, offset+this_len);
-       if (ret == AOP_TRUNCATED_PAGE) {
+       if (!ret) {
+               /*
+                * Return the number of bytes written and mark page as
+                * accessed, we are now done!
+                */
+               ret = this_len;
+               mark_page_accessed(page);
+               balance_dirty_pages_ratelimited(mapping);
+       } else if (ret == AOP_TRUNCATED_PAGE) {
                page_cache_release(page);
                goto find_page;
-       } else if (ret)
-               goto out;
-
-       /*
-        * Return the number of bytes written.
-        */
-       ret = this_len;
-       mark_page_accessed(page);
-       balance_dirty_pages_ratelimited(mapping);
+       }
 out:
-       if (!(buf->flags & PIPE_BUF_FLAG_STOLEN))
-               page_cache_release(page);
-
+       page_cache_release(page);
        unlock_page(page);
 out_nomem:
        buf->ops->unmap(info, buf);

diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 0008d4bd4059622972dce43aaf5d78acbdab0b80..3130977fc6ab4e5ff02a638df841ab75eddc1843 100644 (file)
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -5,8 +5,7 @@
 
 #define PIPE_BUFFERS (16)
 
-#define PIPE_BUF_FLAG_STOLEN   0x01
-#define PIPE_BUF_FLAG_LRU      0x02
+#define PIPE_BUF_FLAG_LRU      0x01
 
 struct pipe_buffer {
        struct page *page;