Hi Kim, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk.kim@xxxxxxxxxxx] > Sent: Monday, December 23, 2013 9:26 AM > To: Chao Yu > Cc: linux-fsdevel@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check filename length in recover_dentry > > 2013-12-21 (토), 18:01 +0800, Chao Yu: > > In current flow, we will get Null return value of f2fs_find_entry in > > recover_dentry when name.len is bigger than F2FS_NAME_LEN, and then we > > still add this inode into its dir entry. > > To avoid this situation, we must check filename length before we use it. > > > > Another point is that we could remove the code of checking filename length > > In f2fs_find_entry, because f2fs_lookup will be called previously to ensure of > > validity of filename length. > > > > Signed-off-by: Chao Yu <chao2.yu@xxxxxxxxxxx> > > --- > > fs/f2fs/dir.c | 3 --- > > fs/f2fs/recovery.c | 5 +++++ > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > index 0cc26ba..3f3b661 100644 > > --- a/fs/f2fs/dir.c > > +++ b/fs/f2fs/dir.c > > @@ -190,9 +190,6 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir, > > unsigned int max_depth; > > unsigned int level; > > > > - if (unlikely(namelen > F2FS_NAME_LEN)) > > - return NULL; > > - > > if (npages == 0) > > return NULL; > > > > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c > > index a3f4542..fdd175b 100644 > > --- a/fs/f2fs/recovery.c > > +++ b/fs/f2fs/recovery.c > > @@ -62,6 +62,11 @@ static int recover_dentry(struct page *ipage, struct inode *inode) > > > > name.len = le32_to_cpu(raw_inode->i_namelen); > > name.name = raw_inode->i_name; > > + > > + if (unlikely(name.len > F2FS_NAME_LEN)) { > > + err = -ENAMETOOLONG; > > + goto out; > > + } > > Have you seen this before? Not yet. > This is a trivial bug case, so, if you have got this bug, we should fix > the bug first instead of adding any workaround patch. What I worry about is that not only f2fs bug lead to this trivial problem, but also other program with operation in raw disk could do this. > Let's add WARN_ON() at least. Alright. Thanks. > Thanks, > > > retry: > > de = f2fs_find_entry(dir, &name, &page); > > if (de && inode->i_ino == le32_to_cpu(de->ino)) > > -- > Jaegeuk Kim > Samsung -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
