Hi Jaegeuk, Yunlei,
On 2016/4/26 8:07, Jaegeuk Kim wrote:
> Let's consider a race condition between f2fs_add_regular_entry and
> find_target_dentry.
>
> 1.
> - f2fs_add_regular_entry updated len: 24 first.
> |
> Bits: 0 0 0 1
> Lens: 24 0 0 3 (name: foo)
> |->
> - find_target_dentry checks the first bit to find "foo", then ++pointer.
>
> 2.
> - f2fs_add_regular_entry updates bits.
> |>|>|
> Bits: 1 1 1 1
> Lens: 24 0 0 3 (name: foo)
> |
> - find_target_dentry is checking second bit, but it's len is zero, which
> makes the process being terminated.
As Pengyang reminded, there are no racing condition between find_target_dentry
and f2fs_add_regular_entry since i_mutex lock make each of operations being
atomical. So seems above condition can not happen.
But still we should handle dirent with zero-sized length correctly, as it may
cause deadloop. So how do you think of following patch?
From: Chao Yu <yuch...@huawei.com>
Subject: [PATCH] f2fs: be aware of invalid filename length
The filename length in dirent of may become zero-sized after random junk
data injection, once encounter such dirent, find_target_dentry or
f2fs_add_inline_entries will run into an infinite loop. So let f2fs being
aware of that to avoid deadloop.
Signed-off-by: Chao Yu <yuch...@huawei.com>
---
fs/f2fs/dir.c | 14 +++++---------
fs/f2fs/inline.c | 14 ++++++--------
2 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index e90380d..3b1c14e 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -101,11 +101,6 @@ static struct f2fs_dir_entry *find_in_block(struct page
*dentry_page,
else
kunmap(dentry_page);
- /*
- * For the most part, it should be a bug when name_len is zero.
- * We stop here for figuring out where the bugs has occurred.
- */
- f2fs_bug_on(F2FS_P_SB(dentry_page), d.max < 0);
return de;
}
@@ -130,6 +125,11 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,
de = &d->dentry[bit_pos];
+ if (unlikely(!de->name_len)) {
+ bit_pos++;
+ continue;
+ }
+
/* encrypted case */
de_name.name = d->filename[bit_pos];
de_name.len = le16_to_cpu(de->name_len);
@@ -147,10 +147,6 @@ struct f2fs_dir_entry *find_target_dentry(struct
fscrypt_name *fname,
*max_slots = max_len;
max_len = 0;
- /* remain bug on condition */
- if (unlikely(!de->name_len))
- d->max = -1;
-
bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
}
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index 7720565..e61084c 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -303,11 +303,6 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode
*dir,
else
f2fs_put_page(ipage, 0);
- /*
- * For the most part, it should be a bug when name_len is zero.
- * We stop here for figuring out where the bugs has occurred.
- */
- f2fs_bug_on(sbi, d.max < 0);
return de;
}
@@ -437,6 +432,12 @@ static int f2fs_add_inline_entries(struct inode *dir,
}
de = &d.dentry[bit_pos];
+
+ if (unlikely(!de->name_len)) {
+ bit_pos++;
+ continue;
+ }
+
new_name.name = d.filename[bit_pos];
new_name.len = de->name_len;
@@ -448,9 +449,6 @@ static int f2fs_add_inline_entries(struct inode *dir,
if (err)
goto punch_dentry_pages;
- if (unlikely(!de->name_len))
- d.max = -1;
-
bit_pos += GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
}
return 0;
--
2.7.2
