Re: [PATCH RFC 0/2] Btrfs: fix file data corruptions due to lost dirty bits

[David Sterba <dsterba@suse.cz>]

On Fri, Jun 22, 2018 at 05:25:54PM -0400, Chris Mason wrote:
> The bug came here:
> 
> commit a528a24150870c5c16cbbbec69dba7e992b08456
> Author: Souptick Joarder <jrdr.linux@gmail.com>
> Date:   Wed Jun 6 19:54:44 2018 +0530
> 
>      btrfs: change return type of btrfs_page_mkwrite to vm_fault_t
> 
> When page->mapping != mapping, we improperly return success because ret2 
> is zero on goto out_unlock.
> 
> I'm running a fix through a full xfstests and I'll post soon.

The ret/ret2 pattern is IMO used in the wrong way, the ret2 is some
temporary value and it should be set and tested next to each other, not
holding the state accross the function.

The fix I'd propose is to avoid relying on it and add a separate exit
block, similar to out_noreserve:

--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8981,7 +8981,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
                ret = VM_FAULT_SIGBUS;
                goto out_unlock;
        }
-       ret2 = 0;
 
        /* page is wholly or partially inside EOF */
        if (page_start + PAGE_SIZE > size)
@@ -9004,14 +9003,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
        BTRFS_I(inode)->last_log_commit = BTRFS_I(inode)->root->last_log_commit;
 
        unlock_extent_cached(io_tree, page_start, page_end, &cached_state);
-
-out_unlock:
-       if (!ret2) {
-               btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
-               sb_end_pagefault(inode->i_sb);
-               extent_changeset_free(data_reserved);
-               return VM_FAULT_LOCKED;
-       }
        unlock_page(page);
 out:
        btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0));
@@ -9021,6 +9012,12 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
        sb_end_pagefault(inode->i_sb);
        extent_changeset_free(data_reserved);
        return ret;
+
+out_unlock:
+       btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true);
+       sb_end_pagefault(inode->i_sb);
+       extent_changeset_free(data_reserved);
+       return VM_FAULT_LOCKED;
 }
 
 static int btrfs_truncate(struct inode *inode, bool skip_writeback)