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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >   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))
>