lock-xattr-block-before-calculating-checksum

   1 ext4: lock the xattr block before checksuming it
   2
   3 We must lock the xattr block before calculating or verifying the
   4 checksum in order to avoid spurious checksum failures.
   5
   6 https://bugzilla.kernel.org/show_bug.cgi?id=193661
   7
   8 Reported-by: Colin Ian King <colin.king@canonical.com>
   9 Signed-off-by: Theodore Ts'o <tytso@mit.edu>
  10 Cc: stable@vger.kernel.org
  11 ---
  12  fs/ext4/xattr.c | 65 +++++++++++++++++++++++++++++++----------------------------------
  13  1 file changed, 31 insertions(+), 34 deletions(-)
  14
  15 diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
  16 index 67636acf7624..996e7900d4c8 100644
  17 --- a/fs/ext4/xattr.c
  18 +++ b/fs/ext4/xattr.c
  19 @@ -131,31 +131,26 @@ static __le32 ext4_xattr_block_csum(struct inode *inode,
  20  }
  21
  22  static int ext4_xattr_block_csum_verify(struct inode *inode,
  23 -                                       sector_t block_nr,
  24 -                                       struct ext4_xattr_header *hdr)
  25 +                                       struct buffer_head *bh)
  26  {
  27 -       if (ext4_has_metadata_csum(inode->i_sb) &&
  28 -           (hdr->h_checksum != ext4_xattr_block_csum(inode, block_nr, hdr)))
  29 -               return 0;
  30 -       return 1;
  31 -}
  32 -
  33 -static void ext4_xattr_block_csum_set(struct inode *inode,
  34 -                                     sector_t block_nr,
  35 -                                     struct ext4_xattr_header *hdr)
  36 -{
  37 -       if (!ext4_has_metadata_csum(inode->i_sb))
  38 -               return;
  39 +       struct ext4_xattr_header *hdr = BHDR(bh);
  40 +       int ret = 1;
  41
  42 -       hdr->h_checksum = ext4_xattr_block_csum(inode, block_nr, hdr);
  43 +       if (ext4_has_metadata_csum(inode->i_sb)) {
  44 +               lock_buffer(bh);
  45 +               ret = (hdr->h_checksum == ext4_xattr_block_csum(inode,
  46 +                                                       bh->b_blocknr, hdr));
  47 +               unlock_buffer(bh);
  48 +       }
  49 +       return ret;
  50  }
  51
  52 -static inline int ext4_handle_dirty_xattr_block(handle_t *handle,
  53 -                                               struct inode *inode,
  54 -                                               struct buffer_head *bh)
  55 +static void ext4_xattr_block_csum_set(struct inode *inode,
  56 +                                     struct buffer_head *bh)
  57  {
  58 -       ext4_xattr_block_csum_set(inode, bh->b_blocknr, BHDR(bh));
  59 -       return ext4_handle_dirty_metadata(handle, inode, bh);
  60 +       if (ext4_has_metadata_csum(inode->i_sb))
  61 +               BHDR(bh)->h_checksum = ext4_xattr_block_csum(inode,
  62 +                                               bh->b_blocknr, BHDR(bh));
  63  }
  64
  65  static inline const struct xattr_handler *
  66 @@ -233,7 +228,7 @@ ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh)
  67         if (BHDR(bh)->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC) ||
  68             BHDR(bh)->h_blocks != cpu_to_le32(1))
  69                 return -EFSCORRUPTED;
  70 -       if (!ext4_xattr_block_csum_verify(inode, bh->b_blocknr, BHDR(bh)))
  71 +       if (!ext4_xattr_block_csum_verify(inode, bh))
  72                 return -EFSBADCRC;
  73         error = ext4_xattr_check_names(BFIRST(bh), bh->b_data + bh->b_size,
  74                                        bh->b_data);
  75 @@ -618,23 +613,22 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
  76                         }
  77                 }
  78
  79 +               ext4_xattr_block_csum_set(inode, bh);
  80                 /*
  81                  * Beware of this ugliness: Releasing of xattr block references
  82                  * from different inodes can race and so we have to protect
  83                  * from a race where someone else frees the block (and releases
  84                  * its journal_head) before we are done dirtying the buffer. In
  85                  * nojournal mode this race is harmless and we actually cannot
  86 -                * call ext4_handle_dirty_xattr_block() with locked buffer as
  87 +                * call ext4_handle_dirty_metadata() with locked buffer as
  88                  * that function can call sync_dirty_buffer() so for that case
  89                  * we handle the dirtying after unlocking the buffer.
  90                  */
  91                 if (ext4_handle_valid(handle))
  92 -                       error = ext4_handle_dirty_xattr_block(handle, inode,
  93 -                                                             bh);
  94 +                       error = ext4_handle_dirty_metadata(handle, inode, bh);
  95                 unlock_buffer(bh);
  96                 if (!ext4_handle_valid(handle))
  97 -                       error = ext4_handle_dirty_xattr_block(handle, inode,
  98 -                                                             bh);
  99 +                       error = ext4_handle_dirty_metadata(handle, inode, bh);
 100                 if (IS_SYNC(inode))
 101                         ext4_handle_sync(handle);
 102                 dquot_free_block(inode, EXT4_C2B(EXT4_SB(inode->i_sb), 1));
 103 @@ -863,13 +857,14 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 104                                 ext4_xattr_cache_insert(ext4_mb_cache,
 105                                         bs->bh);
 106                         }
 107 +                       ext4_xattr_block_csum_set(inode, bs->bh);
 108                         unlock_buffer(bs->bh);
 109                         if (error == -EFSCORRUPTED)
 110                                 goto bad_block;
 111                         if (!error)
 112 -                               error = ext4_handle_dirty_xattr_block(handle,
 113 -                                                                     inode,
 114 -                                                                     bs->bh);
 115 +                               error = ext4_handle_dirty_metadata(handle,
 116 +                                                                  inode,
 117 +                                                                  bs->bh);
 118                         if (error)
 119                                 goto cleanup;
 120                         goto inserted;
 121 @@ -967,10 +962,11 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 122                                         ce->e_reusable = 0;
 123                                 ea_bdebug(new_bh, "reusing; refcount now=%d",
 124                                           ref);
 125 +                               ext4_xattr_block_csum_set(inode, new_bh);
 126                                 unlock_buffer(new_bh);
 127 -                               error = ext4_handle_dirty_xattr_block(handle,
 128 -                                                                     inode,
 129 -                                                                     new_bh);
 130 +                               error = ext4_handle_dirty_metadata(handle,
 131 +                                                                  inode,
 132 +                                                                  new_bh);
 133                                 if (error)
 134                                         goto cleanup_dquot;
 135                         }
 136 @@ -1020,11 +1016,12 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 137                                 goto getblk_failed;
 138                         }
 139                         memcpy(new_bh->b_data, s->base, new_bh->b_size);
 140 +                       ext4_xattr_block_csum_set(inode, new_bh);
 141                         set_buffer_uptodate(new_bh);
 142                         unlock_buffer(new_bh);
 143                         ext4_xattr_cache_insert(ext4_mb_cache, new_bh);
 144 -                       error = ext4_handle_dirty_xattr_block(handle,
 145 -                                                             inode, new_bh);
 146 +                       error = ext4_handle_dirty_metadata(handle, inode,
 147 +                                                          new_bh);
 148                         if (error)
 149                                 goto cleanup;
 150                 }