Message-Id: <508F1BF9-1F80-4F7D-92A3-D44F82533C61@dilger.ca>
Date:   Thu, 23 Jun 2022 11:01:58 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Ye Bin <yebin10@...wei.com>
Cc:     Theodore Ts'o <tytso@....edu>,
        linux-ext4 <linux-ext4@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>
Subject: Re: [PATCH -next] ext4: avoid remove directory when directory is
 corrupted

On Jun 22, 2022, at 3:02 AM, Ye Bin <yebin10@...wei.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@...wei.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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists