[PATCH] ext4: update last_pos for the case ext4_htree_fill_tree return fail

[PATCH] ext4: update last_pos for the case ext4_htree_fill_tree return fail

[yangerkun]

Or the ls for ext4 dir can run into a deadloop since info->last_pos !=
ctx->pos which will reset the world and start read the entry which has
already got before. Details see below:

1. a dx_dir which has 3 block, block 0 as dx_root block, block 1/2 as
   leaf block which own the ext4_dir_entry_2
2. block 1 read ok and call_filldir which will fill the dirent and update
   the ctx->pos
3. block 2 read fail, but we has already fill some dirent, so we will
   return back to userspace will a positive return val(see ksys_getdents64)
4. the second ext4_dx_readdir will reset the world since info->last_pos
   != ctx->pos, and will also init the curr_hash which pos to block 1
5. So we will read block1 too, and once block2 still read fail, we can
   only fill one dirent because the hash of the entry in block1(besides
   the last one) won't greater than curr_hash
6. this time, we forget update last_pos too since the read for block2
   will fail, and since we has got the one entry, ksys_getdents64 can
   return success
7. Latter we will trapped in a loop with step 4~6

Fix it by update last_pos too once ext4_htree_fill_tree return fail.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 fs/ext4/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index ffb295aa891c..74b172a4adda 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -551,7 +551,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
 	struct dir_private_info *info = file->private_data;
 	struct inode *inode = file_inode(file);
 	struct fname *fname;
-	int	ret;
+	int ret = 0;
 
 	if (!info) {
 		info = ext4_htree_create_dir_info(file, ctx->pos);
@@ -599,7 +599,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
 						   info->curr_minor_hash,
 						   &info->next_hash);
 			if (ret < 0)
-				return ret;
+				goto finished;
 			if (ret == 0) {
 				ctx->pos = ext4_get_htree_eof(file);
 				break;
@@ -630,7 +630,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
 	}
 finished:
 	info->last_pos = ctx->pos;
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static int ext4_release_dir(struct inode *inode, struct file *filp)
-- 
2.31.1

Re: [PATCH] ext4: update last_pos for the case ext4_htree_fill_tree return fail

[Jan Kara]

On Tue 14-09-21 19:14:15, yangerkun wrote:
> Or the ls for ext4 dir can run into a deadloop since info->last_pos !=
> ctx->pos which will reset the world and start read the entry which has
> already got before. Details see below:
> 
> 1. a dx_dir which has 3 block, block 0 as dx_root block, block 1/2 as
>    leaf block which own the ext4_dir_entry_2
> 2. block 1 read ok and call_filldir which will fill the dirent and update
>    the ctx->pos
> 3. block 2 read fail, but we has already fill some dirent, so we will
>    return back to userspace will a positive return val(see ksys_getdents64)
> 4. the second ext4_dx_readdir will reset the world since info->last_pos
>    != ctx->pos, and will also init the curr_hash which pos to block 1
> 5. So we will read block1 too, and once block2 still read fail, we can
>    only fill one dirent because the hash of the entry in block1(besides
>    the last one) won't greater than curr_hash
> 6. this time, we forget update last_pos too since the read for block2
>    will fail, and since we has got the one entry, ksys_getdents64 can
>    return success
> 7. Latter we will trapped in a loop with step 4~6
> 
> Fix it by update last_pos too once ext4_htree_fill_tree return fail.
> 
> Signed-off-by: yangerkun <yangerkun@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/dir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index ffb295aa891c..74b172a4adda 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -551,7 +551,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
>  	struct dir_private_info *info = file->private_data;
>  	struct inode *inode = file_inode(file);
>  	struct fname *fname;
> -	int	ret;
> +	int ret = 0;
>  
>  	if (!info) {
>  		info = ext4_htree_create_dir_info(file, ctx->pos);
> @@ -599,7 +599,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
>  						   info->curr_minor_hash,
>  						   &info->next_hash);
>  			if (ret < 0)
> -				return ret;
> +				goto finished;
>  			if (ret == 0) {
>  				ctx->pos = ext4_get_htree_eof(file);
>  				break;
> @@ -630,7 +630,7 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)
>  	}
>  finished:
>  	info->last_pos = ctx->pos;
> -	return 0;
> +	return ret < 0 ? ret : 0;
>  }
>  
>  static int ext4_release_dir(struct inode *inode, struct file *filp)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: update last_pos for the case ext4_htree_fill_tree return fail

[Theodore Ts'o]

On Tue, 14 Sep 2021 19:14:15 +0800, yangerkun wrote:
> Or the ls for ext4 dir can run into a deadloop since info->last_pos !=
> ctx->pos which will reset the world and start read the entry which has
> already got before. Details see below:
> 
> 1. a dx_dir which has 3 block, block 0 as dx_root block, block 1/2 as
>    leaf block which own the ext4_dir_entry_2
> 2. block 1 read ok and call_filldir which will fill the dirent and update
>    the ctx->pos
> 3. block 2 read fail, but we has already fill some dirent, so we will
>    return back to userspace will a positive return val(see ksys_getdents64)
> 4. the second ext4_dx_readdir will reset the world since info->last_pos
>    != ctx->pos, and will also init the curr_hash which pos to block 1
> 5. So we will read block1 too, and once block2 still read fail, we can
>    only fill one dirent because the hash of the entry in block1(besides
>    the last one) won't greater than curr_hash
> 6. this time, we forget update last_pos too since the read for block2
>    will fail, and since we has got the one entry, ksys_getdents64 can
>    return success
> 7. Latter we will trapped in a loop with step 4~6
> 
> [...]

I rewrote the patch summary and the first paragraph to improve the
English:

    ext4: fix potential infinite loop in ext4_dx_readdir()
    
    When ext4_htree_fill_tree() fails, ext4_dx_readdir() can run into an
    infinite loop since if info->last_pos != ctx->pos this will reset the
    directory scan and reread the failing entry.  For example:

Applied, thanks!

[1/1] ext4: update last_pos for the case ext4_htree_fill_tree return fail
      commit: 42cb447410d024e9d54139ae9c21ea132a8c384c

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>