| Message ID | 20220622090223.682234-1-yebin10@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [-next] ext4: avoid remove directory when directory is corrupted | expand |
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 >
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
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
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,
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; }
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(-)