| Message ID | 5720CB1C.10207@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks better. Merged. :) Thanks, On Wed, Apr 27, 2016 at 10:22:20PM +0800, Chao Yu wrote: > 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 <yuchao0@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 <yuchao0@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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;