[PATCH -next] ext4: avoid remove directory when directory is corrupted

[PATCH -next] ext4: avoid remove directory when directory is corrupted

[Ye Bin]

Now if check directoy entry is corrupted, ext4_empty_dir may return true
then directory will be removed when file system mounted with "errors=continue".
In order not to make things worse just return false when directory is corrupted.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/namei.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 47d0ca4c795b..bc503e3275db 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3066,11 +3066,8 @@ bool ext4_empty_dir(struct inode *inode)
 		de = (struct ext4_dir_entry_2 *) (bh->b_data +
 					(offset & (sb->s_blocksize - 1)));
 		if (ext4_check_dir_entry(inode, NULL, de, bh,
-					 bh->b_data, bh->b_size, offset)) {
-			offset = (offset | (sb->s_blocksize - 1)) + 1;
-			continue;
-		}
-		if (le32_to_cpu(de->inode)) {
+					 bh->b_data, bh->b_size, offset) ||
+		    le32_to_cpu(de->inode)) {
 			brelse(bh);
 			return false;
 		}
-- 
2.31.1

Re: [PATCH -next] ext4: avoid remove directory when directory is corrupted

[Jan Kara]

On Wed 22-06-22 17:02:23, Ye Bin wrote:
> Now if check directoy entry is corrupted, ext4_empty_dir may return true
> then directory will be removed when file system mounted with "errors=continue".
> In order not to make things worse just return false when directory is corrupted.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

OK, looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/namei.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 47d0ca4c795b..bc503e3275db 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3066,11 +3066,8 @@ bool ext4_empty_dir(struct inode *inode)
>  		de = (struct ext4_dir_entry_2 *) (bh->b_data +
>  					(offset & (sb->s_blocksize - 1)));
>  		if (ext4_check_dir_entry(inode, NULL, de, bh,
> -					 bh->b_data, bh->b_size, offset)) {
> -			offset = (offset | (sb->s_blocksize - 1)) + 1;
> -			continue;
> -		}
> -		if (le32_to_cpu(de->inode)) {
> +					 bh->b_data, bh->b_size, offset) ||
> +		    le32_to_cpu(de->inode)) {
>  			brelse(bh);
>  			return false;
>  		}
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: avoid remove directory when directory is corrupted

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

On Jun 22, 2022, at 3:02 AM, Ye Bin <yebin10@huawei.com> wrote:
> 
> Now if check directoy entry is corrupted, ext4_empty_dir may return true
> then directory will be removed when file system mounted with "errors=continue".
> In order not to make things worse just return false when directory is corrupted.

This will make corrupted directories undeletable, which might cause problems
for applications also (e.g. tar or rsync always hitting errors when walking
the tree) and the user may prefer to delete the directory and recreate it
rather than having a permanent error in the filesystem.

With your patch it would always return "false" if a directory block hits a
corrupted entry instead of checking the rest of the blocks in the directory.
Since e2fsck would put the entries from the broken block into lost+found,
it isn't clear that the full/empty decision should be made by the presence
of a corrupted leaf block either way.

Looking at the ext4_empty_dir() code, it looks like there are a few cases
where it might return "true" when the directory actually has entries in it,
but your patch doesn't address those.  IMHO, errors like the absence of "."
and ".." should *NOT* cause the directory to be marked "empty", but it should
continue checking blocks to see if there are valid entries.  However, Jan
added these checks in 64d4ce8923 ("ext4: fix ext4_empty_dir() for directories
with holes") to avoid looping forever when i_size is large and there are no
allocated blocks in the directory, so they shouldn't just be removed, but
they also do not fix the problem if i_size is corrupt but the first block of
the inode is valid.


It might make sense to change ext4_empty_dir() to iterate only leaf blocks
actually allocated in the inode, rather than walking the whole of i_size by
offset?  That would avoid the "spin forever on a huge sparse inode" problem
that was the original reason for the addition of "." and ".." checks, and
give a better determination of whether the directory is actually empty.

If there are only corrupt blocks or holes in the directory there is no reason
*not* to delete it, but if there *are* valid entries (even if "." or ".." are
missing) then the directory should not be deletable, since e2fsck will repair
missing "." and ".." without clobbering the whole directory.

Cheers, Andreas


> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/namei.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 47d0ca4c795b..bc503e3275db 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3066,11 +3066,8 @@ bool ext4_empty_dir(struct inode *inode)
> 		de = (struct ext4_dir_entry_2 *) (bh->b_data +
> 					(offset & (sb->s_blocksize - 1)));
> 		if (ext4_check_dir_entry(inode, NULL, de, bh,
> -					 bh->b_data, bh->b_size, offset)) {
> -			offset = (offset | (sb->s_blocksize - 1)) + 1;
> -			continue;
> -		}
> -		if (le32_to_cpu(de->inode)) {
> +					 bh->b_data, bh->b_size, offset) ||
> +		    le32_to_cpu(de->inode)) {
> 			brelse(bh);
> 			return false;
> 		}
> --
> 2.31.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH -next] ext4: avoid remove directory when directory is corrupted

[Jan Kara]

On Thu 23-06-22 11:01:58, Andreas Dilger wrote:
> On Jun 22, 2022, at 3:02 AM, Ye Bin <yebin10@huawei.com> wrote:
> > 
> > Now if check directoy entry is corrupted, ext4_empty_dir may return true
> > then directory will be removed when file system mounted with "errors=continue".
> > In order not to make things worse just return false when directory is corrupted.
> 
> This will make corrupted directories undeletable, which might cause problems
> for applications also (e.g. tar or rsync always hitting errors when walking
> the tree) and the user may prefer to delete the directory and recreate it
> rather than having a permanent error in the filesystem.

Well, I guess an argument could be made that in such case users should
rather run e2fsck and *that* should remove the error from the filesystem.
It isn't like we allow other metadata corruptions to be papered over by
hiding them. I know we have this policy "corrupted dirs can be deleted"
since basically forever but in retrospection it does not seem particularly
good one to me.

> With your patch it would always return "false" if a directory block hits a
> corrupted entry instead of checking the rest of the blocks in the directory.
> Since e2fsck would put the entries from the broken block into lost+found,
> it isn't clear that the full/empty decision should be made by the presence
> of a corrupted leaf block either way.
> 
> Looking at the ext4_empty_dir() code, it looks like there are a few cases
> where it might return "true" when the directory actually has entries in it,
> but your patch doesn't address those.  IMHO, errors like the absence of "."
> and ".." should *NOT* cause the directory to be marked "empty", but it should
> continue checking blocks to see if there are valid entries.  However, Jan
> added these checks in 64d4ce8923 ("ext4: fix ext4_empty_dir() for directories
> with holes") to avoid looping forever when i_size is large and there are no
> allocated blocks in the directory, so they shouldn't just be removed, but
> they also do not fix the problem if i_size is corrupt but the first block of
> the inode is valid.
> 
> 
> It might make sense to change ext4_empty_dir() to iterate only leaf blocks
> actually allocated in the inode, rather than walking the whole of i_size by
> offset?  That would avoid the "spin forever on a huge sparse inode" problem
> that was the original reason for the addition of "." and ".." checks, and
> give a better determination of whether the directory is actually empty.
> 
> If there are only corrupt blocks or holes in the directory there is no reason
> *not* to delete it, but if there *are* valid entries (even if "." or ".." are
> missing) then the directory should not be deletable, since e2fsck will repair
> missing "." and ".." without clobbering the whole directory.

So I agree this would be a sane option as well but honestly I'm not sure
the complications are worth it. IMHO "corrupted dir is undeletable" is OK
policy because simple things are harder to break ;)...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH -next] ext4: avoid remove directory when directory is corrupted

[Theodore Ts'o]

On Wed, 22 Jun 2022 17:02:23 +0800, Ye Bin wrote:
> Now if check directoy entry is corrupted, ext4_empty_dir may return true
> then directory will be removed when file system mounted with "errors=continue".
> In order not to make things worse just return false when directory is corrupted.
> 
> 

Applied, thanks!

[1/1] ext4: avoid remove directory when directory is corrupted
      commit: 39aa54792eaef404150d323709252f048aa5b6ff

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