Re: [Cluster-devel] [PATCH v5 1/3] iomap: Fix use-after-free error in page_done callback

[Christoph Hellwig]

This looks ok to me, holding the page over the i_size update
and mark_inode_dirty should be ok.  But I think it would be a lot
cleaner if rebased ontop of this cleanup, which you could add to the
front of the series:

---
>From 908dbc5e7c26035f992fef84886976e0cda10b98 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <h...@lst.de>
Date: Sat, 27 Apr 2019 08:13:38 +0200
Subject: iomap: cleanup __generic_write_end calling conventions

Move the call to __generic_write_end into the common code flow instead
of duplicating it in each of the three branches.  This requires open
coding the generic_write_end for the buffer_head case.

Signed-off-by: Christoph Hellwig <h...@lst.de>
---
 fs/iomap.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index abdd18e404f8..cfc8a10b3fd8 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -738,13 +738,11 @@ __iomap_write_end(struct inode *inode, loff_t pos, 
unsigned len,
         * uptodate page as a zero-length write, and force the caller to redo
         * the whole thing.
         */
-       if (unlikely(copied < len && !PageUptodate(page))) {
-               copied = 0;
-       } else {
-               iomap_set_range_uptodate(page, offset_in_page(pos), len);
-               iomap_set_page_dirty(page);
-       }
-       return __generic_write_end(inode, pos, copied, page);
+       if (unlikely(copied < len && !PageUptodate(page)))
+               return 0;
+       iomap_set_range_uptodate(page, offset_in_page(pos), len);
+       iomap_set_page_dirty(page);
+       return copied;
 }
 
 static int
@@ -761,7 +759,6 @@ iomap_write_end_inline(struct inode *inode, struct page 
*page,
        kunmap_atomic(addr);
 
        mark_inode_dirty(inode);
-       __generic_write_end(inode, pos, copied, page);
        return copied;
 }
 
@@ -774,12 +771,13 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned 
len,
        if (iomap->type == IOMAP_INLINE) {
                ret = iomap_write_end_inline(inode, page, iomap, pos, copied);
        } else if (iomap->flags & IOMAP_F_BUFFER_HEAD) {
-               ret = generic_write_end(NULL, inode->i_mapping, pos, len,
-                               copied, page, NULL);
+               ret = block_write_end(NULL, inode->i_mapping, pos, len, copied,
+                               page, NULL);
        } else {
                ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
        }
 
+       ret = __generic_write_end(inode, pos, ret, page);
        if (iomap->page_done)
                iomap->page_done(inode, pos, copied, page, iomap);
 
-- 
2.20.1