[PATCH 0/6 linux-next] ext4: fix extent leaking and clean-up

[PATCH 0/6 linux-next] ext4: fix extent leaking and clean-up

[Fabian Frederick]

Last patch of this small patchset fixes an extent path memory leak.
The rest is some clean-up.

Fabian Frederick (6):
  ext4: avoid EXT4_INODE_EXTENTS double checking
  ext4: remove unneeded test in ext4_alloc_file_blocks()
  ext4: create EXT4_MAX_BLOCKS() macro
  ext4: use bool for check in ext4_ext_space_()
  ext4: remove unused definition
  ext4: fix memory leak in ext4_insert_range()

 fs/ext4/ext4.h    |  3 +++
 fs/ext4/extents.c | 70 ++++++++++++++++++++-----------------------------------
 fs/ext4/file.c    |  3 +--
 fs/ext4/ioctl.c   |  2 --
 4 files changed, 29 insertions(+), 49 deletions(-)

-- 
2.8.1

[PATCH 1/6 linux-next] ext4: avoid EXT4_INODE_EXTENTS double checking

[Fabian Frederick]

ext4_collapse_range() and ext4_insert_range()
already checked inode flag at the beginning of function.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/extents.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d7ccb7f..5d9f99a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5506,12 +5506,6 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 		goto out_mutex;
 	}
 
-	/* Currently just for extent based files */
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
-		ret = -EOPNOTSUPP;
-		goto out_mutex;
-	}
-
 	/* Wait for existing dio to complete */
 	ext4_inode_block_unlocked_dio(inode);
 	inode_dio_wait(inode);
@@ -5643,11 +5637,6 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
 	}
 
 	inode_lock(inode);
-	/* Currently just for extent based files */
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
-		ret = -EOPNOTSUPP;
-		goto out_mutex;
-	}
 
 	/* Check for wrap through zero */
 	if (inode->i_size + len > inode->i_sb->s_maxbytes) {
-- 
2.8.1

[PATCH 2/6 linux-next] ext4: remove unneeded test in ext4_alloc_file_blocks()

[Fabian Frederick]

ext4_alloc_file_blocks() is called from ext4_zero_range() and
ext4_fallocate() both already testing EXT4_INODE_EXTENTS
We can call ext_depth(inode) unconditionnally.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/extents.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5d9f99a..4f1cca8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4693,13 +4693,7 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
 	 * credits to insert 1 extent into extent tree
 	 */
 	credits = ext4_chunk_trans_blocks(inode, len);
-	/*
-	 * We can only call ext_depth() on extent based inodes
-	 */
-	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		depth = ext_depth(inode);
-	else
-		depth = -1;
+	depth = ext_depth(inode);
 
 retry:
 	while (ret >= 0 && len) {
-- 
2.8.1

[PATCH 3/6 linux-next] ext4: create EXT4_MAX_BLOCKS() macro

[Fabian Frederick]

Create a macro to calculate length + offset -> maximum blocks
This adds more readability.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/ext4.h    |  3 +++
 fs/ext4/extents.c | 15 +++------------
 fs/ext4/file.c    |  3 +--
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ea31931..6e98f3f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -262,6 +262,9 @@ struct ext4_io_submit {
 				 (s)->s_first_ino)
 #endif
 #define EXT4_BLOCK_ALIGN(size, blkbits)		ALIGN((size), (1 << (blkbits)))
+#define EXT4_MAX_BLOCKS(size, offset, blkbits) \
+	((EXT4_BLOCK_ALIGN(size + offset, blkbits) >> blkbits) - (offset >> \
+								  blkbits))
 
 /* Translate a block number to a cluster number */
 #define EXT4_B2C(sbi, blk)	((blk) >> (sbi)->s_cluster_bits)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 4f1cca8..ac54907 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4960,13 +4960,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 
 	trace_ext4_fallocate_enter(inode, offset, len, mode);
 	lblk = offset >> blkbits;
-	/*
-	 * We can't just convert len to max_blocks because
-	 * If blocksize = 4096 offset = 3072 and len = 2048
-	 */
-	max_blocks = (EXT4_BLOCK_ALIGN(len + offset, blkbits) >> blkbits)
-		- lblk;
 
+	max_blocks = EXT4_MAX_BLOCKS(len, offset, blkbits);
 	flags = EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT;
 	if (mode & FALLOC_FL_KEEP_SIZE)
 		flags |= EXT4_GET_BLOCKS_KEEP_SIZE;
@@ -5029,12 +5024,8 @@ int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
 	unsigned int credits, blkbits = inode->i_blkbits;
 
 	map.m_lblk = offset >> blkbits;
-	/*
-	 * We can't just convert len to max_blocks because
-	 * If blocksize = 4096 offset = 3072 and len = 2048
-	 */
-	max_blocks = ((EXT4_BLOCK_ALIGN(len + offset, blkbits) >> blkbits) -
-		      map.m_lblk);
+	max_blocks = EXT4_MAX_BLOCKS(len, offset, blkbits);
+
 	/*
 	 * This is somewhat ugly but the idea is clear: When transaction is
 	 * reserved, everything goes into it. Otherwise we rather start several
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 261ac37..34acda7 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -144,8 +144,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			int err, len;
 
 			map.m_lblk = pos >> blkbits;
-			map.m_len = (EXT4_BLOCK_ALIGN(pos + length, blkbits) >> blkbits)
-				- map.m_lblk;
+			map.m_len = EXT4_MAX_BLOCKS(length, pos, blkbits);
 			len = map.m_len;
 
 			err = ext4_map_blocks(NULL, inode, &map, 0);
-- 
2.8.1

[PATCH 4/6 linux-next] ext4: use bool for check in ext4_ext_space_()

[Fabian Frederick]

check is used in 0/1 context.

Also use unsigned int instead of unsigned (checkpatch warning)

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/extents.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ac54907..5b0913d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -241,7 +241,7 @@ ext4_ext_new_meta_block(handle_t *handle, struct inode *inode,
 	return newblock;
 }
 
-static inline int ext4_ext_space_block(struct inode *inode, int check)
+static inline int ext4_ext_space_block(struct inode *inode, bool check)
 {
 	int size;
 
@@ -254,7 +254,7 @@ static inline int ext4_ext_space_block(struct inode *inode, int check)
 	return size;
 }
 
-static inline int ext4_ext_space_block_idx(struct inode *inode, int check)
+static inline int ext4_ext_space_block_idx(struct inode *inode, bool check)
 {
 	int size;
 
@@ -267,7 +267,7 @@ static inline int ext4_ext_space_block_idx(struct inode *inode, int check)
 	return size;
 }
 
-static inline int ext4_ext_space_root(struct inode *inode, int check)
+static inline int ext4_ext_space_root(struct inode *inode, bool check)
 {
 	int size;
 
@@ -363,14 +363,14 @@ ext4_ext_max_entries(struct inode *inode, int depth)
 
 	if (depth == ext_depth(inode)) {
 		if (depth == 0)
-			max = ext4_ext_space_root(inode, 1);
+			max = ext4_ext_space_root(inode, true);
 		else
-			max = ext4_ext_space_root_idx(inode, 1);
+			max = ext4_ext_space_root_idx(inode, true);
 	} else {
 		if (depth == 0)
-			max = ext4_ext_space_block(inode, 1);
+			max = ext4_ext_space_block(inode, true);
 		else
-			max = ext4_ext_space_block_idx(inode, 1);
+			max = ext4_ext_space_block_idx(inode, true);
 	}
 
 	return max;
@@ -864,7 +864,7 @@ int ext4_ext_tree_init(handle_t *handle, struct inode *inode)
 	eh->eh_depth = 0;
 	eh->eh_entries = 0;
 	eh->eh_magic = EXT4_EXT_MAGIC;
-	eh->eh_max = cpu_to_le16(ext4_ext_space_root(inode, 0));
+	eh->eh_max = cpu_to_le16(ext4_ext_space_root(inode, false));
 	ext4_mark_inode_dirty(handle, inode);
 	return 0;
 }
@@ -1109,7 +1109,7 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 
 	neh = ext_block_hdr(bh);
 	neh->eh_entries = 0;
-	neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, 0));
+	neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, false));
 	neh->eh_magic = EXT4_EXT_MAGIC;
 	neh->eh_depth = 0;
 
@@ -1183,7 +1183,8 @@ static int ext4_ext_split(handle_t *handle, struct inode *inode,
 		neh = ext_block_hdr(bh);
 		neh->eh_entries = cpu_to_le16(1);
 		neh->eh_magic = EXT4_EXT_MAGIC;
-		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode, 0));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode,
+								   false));
 		neh->eh_depth = cpu_to_le16(depth - i);
 		fidx = EXT_FIRST_INDEX(neh);
 		fidx->ei_block = border;
@@ -1310,9 +1311,10 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 	/* old root could have indexes or leaves
 	 * so calculate e_max right way */
 	if (ext_depth(inode))
-		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode, 0));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block_idx(inode,
+								   false));
 	else
-		neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, 0));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_block(inode, false));
 	neh->eh_magic = EXT4_EXT_MAGIC;
 	ext4_extent_block_csum_set(inode, neh);
 	set_buffer_uptodate(bh);
@@ -1328,7 +1330,8 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
 	ext4_idx_store_pblock(EXT_FIRST_INDEX(neh), newblock);
 	if (neh->eh_depth == 0) {
 		/* Root extent block becomes index block */
-		neh->eh_max = cpu_to_le16(ext4_ext_space_root_idx(inode, 0));
+		neh->eh_max = cpu_to_le16(ext4_ext_space_root_idx(inode,
+								  false));
 		EXT_FIRST_INDEX(neh)->ei_block =
 			EXT_FIRST_EXTENT(neh)->ee_block;
 	}
@@ -1816,7 +1819,7 @@ static void ext4_ext_try_to_merge_up(handle_t *handle,
 				     struct ext4_ext_path *path)
 {
 	size_t s;
-	unsigned max_root = ext4_ext_space_root(inode, 0);
+	unsigned int max_root = ext4_ext_space_root(inode, false);
 	ext4_fsblk_t blk;
 
 	if ((path[0].p_depth != 1) ||
@@ -3053,7 +3056,7 @@ again:
 		if (err == 0) {
 			ext_inode_hdr(inode)->eh_depth = 0;
 			ext_inode_hdr(inode)->eh_max =
-				cpu_to_le16(ext4_ext_space_root(inode, 0));
+				cpu_to_le16(ext4_ext_space_root(inode, false));
 			err = ext4_ext_dirty(handle, inode, path);
 		}
 	}
-- 
2.8.1

[PATCH 5/6 linux-next] ext4: remove unused definition

[Fabian Frederick]

MAX_32_NUM isn't used in ext4

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 10686fd..5a708c87 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -19,8 +19,6 @@
 #include "ext4_jbd2.h"
 #include "ext4.h"
 
-#define MAX_32_NUM ((((unsigned long long) 1) << 32) - 1)
-
 /**
  * Swap memory between @a and @b for @len bytes.
  *
-- 
2.8.1

[PATCH 6/6 linux-next] ext4: fix memory leak in ext4_insert_range()

[Fabian Frederick]

Running xfstests generic/013 with kmemleak gives the following:

unreferenced object 0xffff8801d3d27de0 (size 96):
  comm "fsstress", pid 4941, jiffies 4294860168 (age 53.485s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff818eaaf3>] kmemleak_alloc+0x23/0x40
    [<ffffffff81179805>] __kmalloc+0xf5/0x1d0
    [<ffffffff8122ef5c>] ext4_find_extent+0x1ec/0x2f0
    [<ffffffff8123530c>] ext4_insert_range+0x34c/0x4a0
    [<ffffffff81235942>] ext4_fallocate+0x4e2/0x8b0
    [<ffffffff81181334>] vfs_fallocate+0x134/0x210
    [<ffffffff8118203f>] SyS_fallocate+0x3f/0x60
    [<ffffffff818efa9b>] entry_SYSCALL_64_fastpath+0x13/0x8f
    [<ffffffffffffffff>] 0xffffffffffffffff

Problem seems mitigated by dropping refs and freeing path
when there's no path[depth].p_ext

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/ext4/extents.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5b0913d..2774df4 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5711,6 +5711,9 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
 			up_write(&EXT4_I(inode)->i_data_sem);
 			goto out_stop;
 		}
+	} else {
+		ext4_ext_drop_refs(path);
+		kfree(path);
 	}
 
 	ret = ext4_es_remove_extent(inode, offset_lblk,
-- 
2.8.1

Re: [PATCH 6/6 linux-next] ext4: fix memory leak in ext4_insert_range()

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:20PM +0200, Fabian Frederick wrote:
> Running xfstests generic/013 with kmemleak gives the following:
> 
> unreferenced object 0xffff8801d3d27de0 (size 96):
>   comm "fsstress", pid 4941, jiffies 4294860168 (age 53.485s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff818eaaf3>] kmemleak_alloc+0x23/0x40
>     [<ffffffff81179805>] __kmalloc+0xf5/0x1d0
>     [<ffffffff8122ef5c>] ext4_find_extent+0x1ec/0x2f0
>     [<ffffffff8123530c>] ext4_insert_range+0x34c/0x4a0
>     [<ffffffff81235942>] ext4_fallocate+0x4e2/0x8b0
>     [<ffffffff81181334>] vfs_fallocate+0x134/0x210
>     [<ffffffff8118203f>] SyS_fallocate+0x3f/0x60
>     [<ffffffff818efa9b>] entry_SYSCALL_64_fastpath+0x13/0x8f
>     [<ffffffffffffffff>] 0xffffffffffffffff
> 
> Problem seems mitigated by dropping refs and freeing path
> when there's no path[depth].p_ext
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, thanks.

					- Ted

Re: [PATCH 1/6 linux-next] ext4: avoid EXT4_INODE_EXTENTS double checking

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:15PM +0200, Fabian Frederick wrote:
> ext4_collapse_range() and ext4_insert_range()
> already checked inode flag at the beginning of function.
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Actually, these checks are required since the check at the beginning
are done before taking the inode lock.

One could argue that we should get rid of the first check, since we
don't need to optimize for the error case.  But removing the second
check is definitely wrong.

Cheers,

					- Ted

Re: [PATCH 2/6 linux-next] ext4: remove unneeded test in ext4_alloc_file_blocks()

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:16PM +0200, Fabian Frederick wrote:
> ext4_alloc_file_blocks() is called from ext4_zero_range() and
> ext4_fallocate() both already testing EXT4_INODE_EXTENTS
> We can call ext_depth(inode) unconditionnally.
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, although I also added a BUG_ON() check to make sure
ext4_alloc_file_blocks() won't get called with an indirect-mapped
inode in the future.

				- Ted

Re: [PATCH 3/6 linux-next] ext4: create EXT4_MAX_BLOCKS() macro

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:17PM +0200, Fabian Frederick wrote:
> Create a macro to calculate length + offset -> maximum blocks
> This adds more readability.
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, thanks.

					- Ted

Re: [PATCH 4/6 linux-next] ext4: use bool for check in ext4_ext_space_()

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:18PM +0200, Fabian Frederick wrote:
> check is used in 0/1 context.
> 
> Also use unsigned int instead of unsigned (checkpatch warning)
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Dropped; this is a checkpatch-only style patch.

						- Ted

Re: [PATCH 5/6 linux-next] ext4: remove unused definition

[Theodore Ts'o]

On Wed, Aug 24, 2016 at 10:03:19PM +0200, Fabian Frederick wrote:
> MAX_32_NUM isn't used in ext4
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, thanks.

					- Ted