[PATCH] f2fs: reject invalid recovered filename lengths

[PATCH] f2fs: reject invalid recovered filename lengths

[Wenjie Qi]

Recovery uses raw_inode->i_namelen directly when rebuilding fsynced
dentries. A zero-length name uses no dentry slots, so recovery can
report success without recreating the dentry.

Treat zero-length and oversized recovered names as corruption, mark
NEED_FSCK, and stop recovery with -EFSCORRUPTED.

Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
---
 fs/f2fs/recovery.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 89af8407b667..70cf99efcb3c 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -126,8 +126,8 @@ static int init_recovered_filename(const struct inode *dir,
 	fname->disk_name.len = le32_to_cpu(raw_inode->i_namelen);
 	fname->disk_name.name = raw_inode->i_name;
 
-	if (WARN_ON(fname->disk_name.len > F2FS_NAME_LEN))
-		return -ENAMETOOLONG;
+	if (!fname->disk_name.len || fname->disk_name.len > F2FS_NAME_LEN)
+		return -EFSCORRUPTED;
 
 	if (!IS_ENCRYPTED(dir)) {
 		usr_fname->name = fname->disk_name.name;
@@ -185,8 +185,16 @@ static int recover_dentry(struct inode *inode, struct folio *ifolio,
 
 	dir = entry->inode;
 	err = init_recovered_filename(dir, raw_inode, &fname, &usr_fname);
-	if (err)
+	if (err) {
+		if (err == -EFSCORRUPTED) {
+			f2fs_err(F2FS_I_SB(inode),
+				 "invalid recovered filename length %u for ino %llu",
+				 le32_to_cpu(raw_inode->i_namelen), inode->i_ino);
+			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
+			f2fs_handle_error(F2FS_I_SB(inode), ERROR_CORRUPTED_INODE);
+		}
 		goto out;
+	}
 retry:
 	de = __f2fs_find_entry(dir, &fname, &folio);
 	if (de && inode->i_ino == le32_to_cpu(de->ino))

Re: [PATCH] f2fs: reject invalid recovered filename lengths

[Chao Yu]

On 7/29/26 19:41, Wenjie Qi wrote:
> Recovery uses raw_inode->i_namelen directly when rebuilding fsynced
> dentries. A zero-length name uses no dentry slots, so recovery can
> report success without recreating the dentry.
> 
> Treat zero-length and oversized recovered names as corruption, mark
> NEED_FSCK, and stop recovery with -EFSCORRUPTED.

Do you have a reproducer for this? maybe using inject.f2fs? It will be
better to cover those bugs w/ xfsqa testcase, could you please consider
to add it?

> 
> Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> ---
>   fs/f2fs/recovery.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 89af8407b667..70cf99efcb3c 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -126,8 +126,8 @@ static int init_recovered_filename(const struct inode *dir,
>   	fname->disk_name.len = le32_to_cpu(raw_inode->i_namelen);
>   	fname->disk_name.name = raw_inode->i_name;
>   
> -	if (WARN_ON(fname->disk_name.len > F2FS_NAME_LEN))
> -		return -ENAMETOOLONG;
> +	if (!fname->disk_name.len || fname->disk_name.len > F2FS_NAME_LEN)

unlikely?

relocate error handling here?

Thanks,

> +		return -EFSCORRUPTED;
>   
>   	if (!IS_ENCRYPTED(dir)) {
>   		usr_fname->name = fname->disk_name.name;
> @@ -185,8 +185,16 @@ static int recover_dentry(struct inode *inode, struct folio *ifolio,
>   
>   	dir = entry->inode;
>   	err = init_recovered_filename(dir, raw_inode, &fname, &usr_fname);
> -	if (err)
> +	if (err) {
> +		if (err == -EFSCORRUPTED) {
> +			f2fs_err(F2FS_I_SB(inode),
> +				 "invalid recovered filename length %u for ino %llu",
> +				 le32_to_cpu(raw_inode->i_namelen), inode->i_ino);
> +			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
> +			f2fs_handle_error(F2FS_I_SB(inode), ERROR_CORRUPTED_INODE);
> +		}
>   		goto out;
> +	}
>   retry:
>   	de = __f2fs_find_entry(dir, &fname, &folio);
>   	if (de && inode->i_ino == le32_to_cpu(de->ino))

Re: [PATCH] f2fs: reject invalid recovered filename lengths

[Wenjie Qi]

  Yes, I have a reproducer.

  This one is not naturally triggered by inject.f2fs, since the bug
comes
  from malformed on-disk recovery metadata rather than a runtime fault
  site.

  The reproducer I used is:

  1. create a file with a 255-byte name;
  2. fsync the file only, without fsyncing the parent directory;
  3. crash before checkpoint so the fsynced inode stays in roll-forward data;
  4. patch the recovery log entry on the image and change i_namelen from
     255 to 0;
  5. mount and let recovery run.

  On current code, recovery reports success, but the fsynced file is
not
  recreated after mount.

 I agree this would be better covered by an xfstests case. I can look at
  turning the reproducer into one, though it needs an image-corruption
  step between crash and remount, so it is a bit different from
  inject.f2fs-based tests.

  For the code, I can add unlikely() to the zero/oversized length check in
  the next version.


On Mon, Aug 3, 2026 at 4:53 PM Chao Yu <chao@kernel.org> wrote:
>
> On 7/29/26 19:41, Wenjie Qi wrote:
> > Recovery uses raw_inode->i_namelen directly when rebuilding fsynced
> > dentries. A zero-length name uses no dentry slots, so recovery can
> > report success without recreating the dentry.
> >
> > Treat zero-length and oversized recovered names as corruption, mark
> > NEED_FSCK, and stop recovery with -EFSCORRUPTED.
>
> Do you have a reproducer for this? maybe using inject.f2fs? It will be
> better to cover those bugs w/ xfsqa testcase, could you please consider
> to add it?
>
> >
> > Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
> > ---
> >   fs/f2fs/recovery.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 89af8407b667..70cf99efcb3c 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -126,8 +126,8 @@ static int init_recovered_filename(const struct inode *dir,
> >       fname->disk_name.len = le32_to_cpu(raw_inode->i_namelen);
> >       fname->disk_name.name = raw_inode->i_name;
> >
> > -     if (WARN_ON(fname->disk_name.len > F2FS_NAME_LEN))
> > -             return -ENAMETOOLONG;
> > +     if (!fname->disk_name.len || fname->disk_name.len > F2FS_NAME_LEN)
>
> unlikely?
>
> relocate error handling here?
>
> Thanks,
>
> > +             return -EFSCORRUPTED;
> >
> >       if (!IS_ENCRYPTED(dir)) {
> >               usr_fname->name = fname->disk_name.name;
> > @@ -185,8 +185,16 @@ static int recover_dentry(struct inode *inode, struct folio *ifolio,
> >
> >       dir = entry->inode;
> >       err = init_recovered_filename(dir, raw_inode, &fname, &usr_fname);
> > -     if (err)
> > +     if (err) {
> > +             if (err == -EFSCORRUPTED) {
> > +                     f2fs_err(F2FS_I_SB(inode),
> > +                              "invalid recovered filename length %u for ino %llu",
> > +                              le32_to_cpu(raw_inode->i_namelen), inode->i_ino);
> > +                     set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
> > +                     f2fs_handle_error(F2FS_I_SB(inode), ERROR_CORRUPTED_INODE);
> > +             }
> >               goto out;
> > +     }
> >   retry:
> >       de = __f2fs_find_entry(dir, &fname, &folio);
> >       if (de && inode->i_ino == le32_to_cpu(de->ino))
>