teach-ext4_ext_find_extent-to-free-path-on-error

   1 ext4: teach ext4_ext_find_extent() to free path on error
   2
   3 Right now, there are a places where it is all to easy to leak memory
   4 on an error path, via a usage like this:
   5
   6         struct ext4_ext_path *path = NULL
   7
   8         while (...) {
   9                 ...
  10                 path = ext4_ext_find_extent(inode, block, path, 0);
  11                 if (IS_ERR(path)) {
  12                         /* oops, if path was non-NULL before the call to
  13                            ext4_ext_find_extent, we've leaked it!  :-(  */
  14                         ...
  15                         return PTR_ERR(path);
  16                 }
  17                 ...
  18         }
  19
  20 Unfortunately, there some code paths where we are doing the following
  21 instead:
  22
  23         path = ext4_ext_find_extent(inode, block, orig_path, 0);
  24
  25 and where it's important that we _not_ free orig_path in the case
  26 where ext4_ext_find_extent() returns an error.
  27
  28 So change the function signature of ext4_ext_find_extent() so that it
  29 takes a struct ext4_ext_path ** for its third argument, and by
  30 default, on an error, it will free the struct ext4_ext_path, and then
  31 zero out the struct ext4_ext_path * pointer.  In order to avoid
  32 causing problems, we add a flag EXT4_EX_NOFREE_ON_ERR which causes
  33 ext4_ext_find_extent() to use the original behavior of forcing the
  34 caller to deal with freeing the original path pointer on the error
  35 case.
  36
  37 The goal is to get rid of EXT4_EX_NOFREE_ON_ERR entirely, but this
  38 allows for a gentle transition and makes the patches easier to verify.
  39
  40 Signed-off-by: Theodore Ts'o <tytso@mit.edu>
  41
  42
  43 ---
  44  fs/ext4/ext4.h        |  3 ++-
  45  fs/ext4/extents.c     | 28 ++++++++++++++++++----------
  46  fs/ext4/move_extent.c |  2 +-
  47  3 files changed, 21 insertions(+), 12 deletions(-)
  48
  49 diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
  50 index 550b4f9..696e51a 100644
  51 --- a/fs/ext4/ext4.h
  52 +++ b/fs/ext4/ext4.h
  53 @@ -582,6 +582,7 @@ enum {
  54   */
  55  #define EXT4_EX_NOCACHE                                0x0800
  56  #define EXT4_EX_FORCE_CACHE                    0x1000
  57 +#define EXT4_EX_NOFREE_ON_ERR                  0x2000
  58
  59  /*
  60   * Flags used by ext4_free_blocks
  61 @@ -2733,7 +2734,7 @@ extern int ext4_ext_insert_extent(handle_t *, struct inode *,
  62                                   struct ext4_ext_path *,
  63                                   struct ext4_extent *, int);
  64  extern struct ext4_ext_path *ext4_ext_find_extent(struct inode *, ext4_lblk_t,
  65 -                                                 struct ext4_ext_path *,
  66 +                                                 struct ext4_ext_path **,
  67                                                   int flags);
  68  extern void ext4_ext_drop_refs(struct ext4_ext_path *);
  69  extern int ext4_ext_check_inode(struct inode *inode);
  70 diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
  71 index bc3b49f..fc76be8 100644
  72 --- a/fs/ext4/extents.c
  73 +++ b/fs/ext4/extents.c
  74 @@ -848,11 +848,13 @@ int ext4_ext_tree_init(handle_t *handle, struct inode *inode)
  75
  76  struct ext4_ext_path *
  77  ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
  78 -                    struct ext4_ext_path *path, int flags)
  79 +                    struct ext4_ext_path **orig_path, int flags)
  80  {
  81         struct ext4_extent_header *eh;
  82         struct buffer_head *bh;
  83 -       short int depth, i, ppos = 0, alloc = 0;
  84 +       struct ext4_ext_path *path = orig_path ? *orig_path : NULL;
  85 +       short int depth, i, ppos = 0;
  86 +       short free_on_err = (flags & EXT4_EX_NOFREE_ON_ERR) == 0;
  87         int ret;
  88
  89         eh = ext_inode_hdr(inode);
  90 @@ -864,7 +866,7 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
  91                                 GFP_NOFS);
  92                 if (unlikely(!path))
  93                         return ERR_PTR(-ENOMEM);
  94 -               alloc = 1;
  95 +               free_on_err = 1;
  96         }
  97         path[0].p_hdr = eh;
  98         path[0].p_bh = NULL;
  99 @@ -916,8 +918,11 @@ ext4_ext_find_extent(struct inode *inode, ext4_lblk_t block,
 100
 101  err:
 102         ext4_ext_drop_refs(path);
 103 -       if (alloc)
 104 +       if (free_on_err) {
 105                 kfree(path);
 106 +               if (orig_path)
 107 +                       *orig_path = NULL;
 108 +       }
 109         return ERR_PTR(ret);
 110  }
 111
 112 @@ -1349,7 +1354,7 @@ repeat:
 113                 ext4_ext_drop_refs(path);
 114                 path = ext4_ext_find_extent(inode,
 115                                     (ext4_lblk_t)le32_to_cpu(newext->ee_block),
 116 -                                   path, gb_flags);
 117 +                                   &path, gb_flags | EXT4_EX_NOFREE_ON_ERR);
 118                 if (IS_ERR(path))
 119                         err = PTR_ERR(path);
 120         } else {
 121 @@ -1362,7 +1367,7 @@ repeat:
 122                 ext4_ext_drop_refs(path);
 123                 path = ext4_ext_find_extent(inode,
 124                                    (ext4_lblk_t)le32_to_cpu(newext->ee_block),
 125 -                                   path, gb_flags);
 126 +                                   &path, gb_flags | EXT4_EX_NOFREE_ON_ERR);
 127                 if (IS_ERR(path)) {
 128                         err = PTR_ERR(path);
 129                         goto out;
 130 @@ -2145,7 +2150,7 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 131                         path = NULL;
 132                 }
 133
 134 -               path = ext4_ext_find_extent(inode, block, path, 0);
 135 +               path = ext4_ext_find_extent(inode, block, &path, 0);
 136                 if (IS_ERR(path)) {
 137                         up_read(&EXT4_I(inode)->i_data_sem);
 138                         err = PTR_ERR(path);
 139 @@ -3319,7 +3324,8 @@ static int ext4_split_extent(handle_t *handle,
 140          * result in split of original leaf or extent zeroout.
 141          */
 142         ext4_ext_drop_refs(path);
 143 -       path = ext4_ext_find_extent(inode, map->m_lblk, path, 0);
 144 +       path = ext4_ext_find_extent(inode, map->m_lblk, &path,
 145 +                                   EXT4_EX_NOFREE_ON_ERR);
 146         if (IS_ERR(path))
 147                 return PTR_ERR(path);
 148         depth = ext_depth(inode);
 149 @@ -3703,7 +3709,8 @@ static int ext4_convert_initialized_extents(handle_t *handle,
 150                 if (err < 0)
 151                         goto out;
 152                 ext4_ext_drop_refs(path);
 153 -               path = ext4_ext_find_extent(inode, map->m_lblk, path, 0);
 154 +               path = ext4_ext_find_extent(inode, map->m_lblk, &path,
 155 +                                           EXT4_EX_NOFREE_ON_ERR);
 156                 if (IS_ERR(path)) {
 157                         err = PTR_ERR(path);
 158                         goto out;
 159 @@ -3775,7 +3782,8 @@ static int ext4_convert_unwritten_extents_endio(handle_t *handle,
 160                 if (err < 0)
 161                         goto out;
 162                 ext4_ext_drop_refs(path);
 163 -               path = ext4_ext_find_extent(inode, map->m_lblk, path, 0);
 164 +               path = ext4_ext_find_extent(inode, map->m_lblk, &path,
 165 +                                           EXT4_EX_NOFREE_ON_ERR);
 166                 if (IS_ERR(path)) {
 167                         err = PTR_ERR(path);
 168                         goto out;
 169 diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
 170 index c8f895b..5e2465a 100644
 171 --- a/fs/ext4/move_extent.c
 172 +++ b/fs/ext4/move_extent.c
 173 @@ -37,7 +37,7 @@ get_ext_path(struct inode *inode, ext4_lblk_t lblock,
 174         int ret = 0;
 175         struct ext4_ext_path *path;
 176
 177 -       path = ext4_ext_find_extent(inode, lblock, *orig_path, EXT4_EX_NOCACHE);
 178 +       path = ext4_ext_find_extent(inode, lblock, orig_path, EXT4_EX_NOCACHE);
 179         if (IS_ERR(path))
 180                 ret = PTR_ERR(path);
 181         else if (path[ext_depth(inode)].p_ext == NULL)