ext4_swap_extents-error-handling

   1 ext4: fix ext4_swap_extents() error handling
   2
   3 If ext4_ext_find_extent() returns an error, we have to clear path1 or
   4 path2 or else we would end up trying to free an ERR_PTR, which would
   5 be bad.
   6
   7 Also eliminate some redundant code and mark the error paths as unlikely()
   8
   9 Signed-off-by: Theodore Ts'o <tytso@mit.edu>
  10
  11 ---
  12  fs/ext4/extents.c | 62 +++++++++++++++++++++++++++++---------------------------------
  13  1 file changed, 29 insertions(+), 33 deletions(-)
  14
  15 diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
  16 index 8555940..bc3b49f 100644
  17 --- a/fs/ext4/extents.c
  18 +++ b/fs/ext4/extents.c
  19 @@ -862,7 +862,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
  20         if (!path) {
  21                 path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 2),
  22                                 GFP_NOFS);
  23 -               if (!path)
  24 +               if (unlikely(!path))
  25                         return ERR_PTR(-ENOMEM);
  26                 alloc = 1;
  27         }
  28 @@ -882,7 +882,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
  29
  30                 bh = read_extent_tree_block(inode, path[ppos].p_block, --i,
  31                                             flags);
  32 -               if (IS_ERR(bh)) {
  33 +               if (unlikely(IS_ERR(bh))) {
  34                         ret = PTR_ERR(bh);
  35                         goto err;
  36                 }
  37 @@ -5551,10 +5551,10 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
  38         BUG_ON(!mutex_is_locked(&inode1->i_mutex));
  39
  40         *erp = ext4_es_remove_extent(inode1, lblk1, count);
  41 -       if (*erp)
  42 +       if (unlikely(*erp))
  43                 return 0;
  44         *erp = ext4_es_remove_extent(inode2, lblk2, count);
  45 -       if (*erp)
  46 +       if (unlikely(*erp))
  47                 return 0;
  48
  49         while (count) {
  50 @@ -5564,20 +5564,24 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
  51                 int split = 0;
  52
  53                 path1 = ext4_ext_find_extent(inode1, lblk1, NULL, EXT4_EX_NOCACHE);
  54 -               if (IS_ERR(path1)) {
  55 +               if (unlikely(IS_ERR(path1))) {
  56                         *erp = PTR_ERR(path1);
  57 -                       break;
  58 +                       path1 = NULL;
  59 +               finish:
  60 +                       count = 0;
  61 +                       goto repeat;
  62                 }
  63                 path2 = ext4_ext_find_extent(inode2, lblk2, NULL, EXT4_EX_NOCACHE);
  64 -               if (IS_ERR(path2)) {
  65 +               if (unlikely(IS_ERR(path2))) {
  66                         *erp = PTR_ERR(path2);
  67 -                       break;
  68 +                       path2 = NULL;
  69 +                       goto finish;
  70                 }
  71                 ex1 = path1[path1->p_depth].p_ext;
  72                 ex2 = path2[path2->p_depth].p_ext;
  73                 /* Do we have somthing to swap ? */
  74                 if (unlikely(!ex2 || !ex1))
  75 -                       break;
  76 +                       goto finish;
  77
  78                 e1_blk = le32_to_cpu(ex1->ee_block);
  79                 e2_blk = le32_to_cpu(ex2->ee_block);
  80 @@ -5599,7 +5603,7 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
  81                                 next2 = e1_blk;
  82                         /* Do we have something to swap */
  83                         if (next1 == EXT_MAX_BLOCKS || next2 == EXT_MAX_BLOCKS)
  84 -                               break;
  85 +                               goto finish;
  86                         /* Move to the rightest boundary */
  87                         len = next1 - lblk1;
  88                         if (len < next2 - lblk2)
  89 @@ -5617,15 +5621,15 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
  90                         split = 1;
  91                         *erp = ext4_force_split_extent_at(handle, inode1,
  92                                                 path1, lblk1, 0);
  93 -                       if (*erp)
  94 -                               break;
  95 +                       if (unlikely(*erp))
  96 +                               goto finish;
  97                 }
  98                 if (e2_blk < lblk2) {
  99                         split = 1;
 100                         *erp = ext4_force_split_extent_at(handle, inode2,
 101                                                 path2,  lblk2, 0);
 102 -                       if (*erp)
 103 -                               break;
 104 +                       if (unlikely(*erp))
 105 +                               goto finish;
 106                 }
 107                 /* ext4_split_extent_at() may retult in leaf extent split,
 108                  * path must to be revalidated. */
 109 @@ -5643,15 +5647,15 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 110                         split = 1;
 111                         *erp = ext4_force_split_extent_at(handle, inode1,
 112                                                 path1, lblk1 + len, 0);
 113 -                       if (*erp)
 114 -                               break;
 115 +                       if (unlikely(*erp))
 116 +                               goto finish;
 117                 }
 118                 if (len != e2_len) {
 119                         split = 1;
 120                         *erp = ext4_force_split_extent_at(handle, inode2,
 121                                                 path2, lblk2 + len, 0);
 122                         if (*erp)
 123 -                               break;
 124 +                               goto finish;
 125                 }
 126                 /* ext4_split_extent_at() may retult in leaf extent split,
 127                  * path must to be revalidated. */
 128 @@ -5660,11 +5664,11 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 129
 130                 BUG_ON(e2_len != e1_len);
 131                 *erp = ext4_ext_get_access(handle, inode1, path1 + path1->p_depth);
 132 -               if (*erp)
 133 -                       break;
 134 +               if (unlikely(*erp))
 135 +                       goto finish;
 136                 *erp = ext4_ext_get_access(handle, inode2, path2 + path2->p_depth);
 137 -               if (*erp)
 138 -                       break;
 139 +               if (unlikely(*erp))
 140 +                       goto finish;
 141
 142                 /* Both extents are fully inside boundaries. Swap it now */
 143                 tmp_ex = *ex1;
 144 @@ -5681,8 +5685,8 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 145                 ext4_ext_try_to_merge(handle, inode1, path1, ex1);
 146                 *erp = ext4_ext_dirty(handle, inode2, path2 +
 147                                       path2->p_depth);
 148 -               if (*erp)
 149 -                       break;
 150 +               if (unlikely(*erp))
 151 +                       goto finish;
 152                 *erp = ext4_ext_dirty(handle, inode1, path1 +
 153                                       path1->p_depth);
 154                 /*
 155 @@ -5691,8 +5695,8 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 156                  * only due to journal error, so full transaction will be
 157                  * aborted anyway.
 158                  */
 159 -               if (*erp)
 160 -                       break;
 161 +               if (unlikely(*erp))
 162 +                       goto finish;
 163                 lblk1 += len;
 164                 lblk2 += len;
 165                 replaced_count += len;
 166 @@ -5710,13 +5714,5 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 167                         path2 = NULL;
 168                 }
 169         }
 170 -       if (path1) {
 171 -               ext4_ext_drop_refs(path1);
 172 -               kfree(path1);
 173 -       }
 174 -       if (path2) {
 175 -               ext4_ext_drop_refs(path2);
 176 -               kfree(path2);
 177 -       }
 178         return replaced_count;
 179  }