btrfs: fix race between dedupe and mmap

Darrick asked how btrfs currently protects against mmap modifying a page
during dedupe, and when I checked I realized it doesn't.  Previously we
did the following dance

lock page ranges in both files
  lock extent
    flush ordered
      validate pages are the same
        dedupe

However Filipe moved us to use the generic checks, which instead does
this dance

lock inode
  flush everything, check for ordered extents
  lock page in both corresponding inodes
    validate pages are the same
  unlock pages
  lock extent
  dedupe

The problem here is we're not doing our normal page lock -> extent lock
-> validate check.  The generic checks assume we've blocked everybody
from modifying the file, which we have with the exception of mmap.

There are two ways we can fix this, and I've chosen the simplest.

The more complicated way is to add a flag to the generic checks to tell
it that we'll do the page verification ourselves.  Then we add back the
checks to btrfs_extent_same() to do the proper lock ordering in order to
validate the pages.

The simpler way to do this is to simply add a mechanism to block mmap
from happening while we're doing dedupe.  I've opted for this strategy,
because it's more straightforward and allows us to continue using the
generic infrastructure.

Ext4 and xfs do not have this problem because they have an inode lock
that they use to block mmap from happening, the i_mmap_sem in ext4's
case and the ilock for xfs.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 28e202e89660f9fdc216f50c6ba30d2a792c9402..468ecf2b72d9be6c623a476bc83278d93484f293 100644 (file)
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -51,6 +51,17 @@ enum {
         * the file range, inode's io_tree).
         */
        BTRFS_INODE_NO_DELALLOC_FLUSH,
+       /*
+        * Set when we are dedupe'ing a file in order to block any mmap writes
+        * from occurring.  This is because we use the generic checking to
+        * validate that the pages are the same, but we do not have the extent
+        * locked at this point to block mmaps.  The trade-off of using the
+        * generic code is we need a separate mechanism to block mmaps in this
+        * case, otherwise we could race and modify pages in between checking if
+        * the pages are the same and locking the extents to do the
+        * deduplication.
+        */
+       BTRFS_INODE_DEDUPE,
 };
 
 /* in memory btrfs inode */
@@ -299,6 +310,28 @@ static inline void btrfs_mod_outstanding_extents(struct btrfs_inode *inode,
                                                  mod);
 }
 
+static inline void btrfs_inode_dedupe(struct btrfs_inode *inode)
+{
+       set_bit(BTRFS_INODE_DEDUPE, &inode->runtime_flags);
+}
+
+static inline int btrfs_inode_dedupe_wait(struct btrfs_inode *inode)
+{
+       return wait_on_bit(&inode->runtime_flags, BTRFS_INODE_DEDUPE,
+                          TASK_INTERRUPTIBLE);
+}
+
+static inline void btrfs_inode_dedupe_done(struct btrfs_inode *inode)
+{
+       clear_bit(BTRFS_INODE_DEDUPE, &inode->runtime_flags);
+       /*
+        * This is necessary because clear_bit doesn't imply a memory barrier,
+        * and we need the memory barrier for wake_up_bit().
+        */
+       smp_mb__after_atomic();
+       wake_up_bit(&inode->runtime_flags, BTRFS_INODE_DEDUPE);
+}
+
 static inline int btrfs_inode_in_log(struct btrfs_inode *inode, u64 generation)
 {
        int ret = 0;

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4f2f1e93275188fcea48215700aa5fb2da1088a9..439db03f86acab52e8c084b300b83d718ac60724 100644 (file)
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8536,7 +8536,22 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 
        ret = VM_FAULT_NOPAGE; /* make the VM retry the fault */
 again:
+       /* We must wait on dedupes to complete. */
+       if (btrfs_inode_dedupe_wait(BTRFS_I(inode)))
+               goto out;
        lock_page(page);
+
+       /*
+        * If we raced and dedupe got set before we locked then we need to retry.
+        * If dedup comes in after this point we're OK because the verification
+        * step must lock this page for the filemap_flush(), so we will block
+        * that step of the dedup until we exit mkwrite, at which point we will
+        * be written out and marked clean again.
+        */
+       if (test_bit(BTRFS_INODE_DEDUPE, &BTRFS_I(inode)->runtime_flags)) {
+               unlock_page(page);
+               goto again;
+       }
        size = i_size_read(inode);
 
        if ((page->mapping != inode->i_mapping) ||

diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index 5413578d2c32d07c9aa74e1393f656ade6d0b19c..1d5acc37fb462035ccdd7e4b7caf908feb81df59 100644 (file)
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -836,10 +836,26 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
        if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
                return -EINVAL;
 
-       if (same_inode)
+       /*
+        * We use btrfs_inode_dedup here to block concurrent mmaps during dedup.
+        * We do this because we use the generic helpers to validate that the
+        * ranges are indeed the same, however the appropriate locking is not
+        * done which makes it racy for us.  The alternative is to stop using
+        * the generic checks and do the pages are the same checks internally
+        * inside btrfs, but since mmap is the only issue here simply block
+        * concurrent mmaps.
+        */
+       if (same_inode) {
                inode_lock(src_inode);
-       else
+               if (remap_flags & REMAP_FILE_DEDUP)
+                       btrfs_inode_dedupe(BTRFS_I(src_inode));
+       } else {
                lock_two_nondirectories(src_inode, dst_inode);
+               if (remap_flags & REMAP_FILE_DEDUP) {
+                       btrfs_inode_dedupe(BTRFS_I(src_inode));
+                       btrfs_inode_dedupe(BTRFS_I(dst_inode));
+               }
+       }
 
        ret = btrfs_remap_file_range_prep(src_file, off, dst_file, destoff,
                                          &len, remap_flags);
@@ -852,10 +868,17 @@ loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
                ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
 
 out_unlock:
-       if (same_inode)
+       if (same_inode) {
                inode_unlock(src_inode);
-       else
+               if (remap_flags & REMAP_FILE_DEDUP)
+                       btrfs_inode_dedupe_done(BTRFS_I(src_inode));
+       } else {
                unlock_two_nondirectories(src_inode, dst_inode);
+               if (remap_flags & REMAP_FILE_DEDUP) {
+                       btrfs_inode_dedupe_done(BTRFS_I(src_inode));
+                       btrfs_inode_dedupe_done(BTRFS_I(dst_inode));
+               }
+       }
 
        return ret < 0 ? ret : len;
 }