Re: [PATCH 2/2] nilfs: check new inode's nlink when rename

On Sat, Aug 8, 2026 at 5:31 PM wangjianjian (C) wrote:
>
> 在 2026/7/30 21:31, Ryusuke Konishi 写道:
> > On Thu, Jul 30, 2026 at 9:32 PM Wang Jianjian wrote:
> >>
> >> When rename, if new inode exists, then its nlink must not be zero.
> >>
> >> Signed-off-by: Wang Jianjian <[email protected]>
> >> ---
> >>   fs/nilfs2/namei.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c
> >> index e2fe95de3d71..db171789a316 100644
> >> --- a/fs/nilfs2/namei.c
> >> +++ b/fs/nilfs2/namei.c
> >> @@ -397,6 +397,11 @@ static int nilfs_rename(struct mnt_idmap *idmap,
> >>                  struct folio *new_folio;
> >>                  struct nilfs_dir_entry *new_de;
> >>
> >> +               if (new_inode->i_nlink == 0) {
> >> +                       err = -EFSCORRUPTED;
> >> +                       goto out_dir;
> >> +               }
> >> +
> >>                  err = -ENOTEMPTY;
> >>                  if (old_is_dir && !nilfs_empty_dir(new_inode))
> >>                          goto out_dir;
> >> --
> >> 2.34.1
> >>
> >
> > Wang Jianjian, regarding this patch - much like the previous one - is
> > this fix related to a Syzbot report?
> >
> > If so, could you please include the appropriate Reported-by and Closes tags?
> >
> > Also, please ensure that the filesystem name in the patch title is
> > "nilfs2" rather than "nilfs" (kernel patch title prefixes generally
> > follow the filesystem name, nilfs2).
> >
> > As for the patch itself: rather than using a stopgap measure to check
> > i_nlink inside nilfs_rename, could you investigate the root cause of
> > the inconsistency? If there is a flaw in the checks when reading the
> > disk inode or a race condition involving link count handling, that is
> > what should be resolved.
> >
> > Additionally, you are returning -EFSCORRUPTED for filesystem
> > corruption, but nilfs2 does not yet utilize this return code.
> > Following the established implementation pattern, when filesystem
> > corruption is detected, you should report it via nilfs_error() (which
> > logs an error message) and return -EIO (while I agree we should
> > eventually switch to -EFSCORRUPTED across the board, doing so here in
> > isolation would create inconsistency).
> >
>
> Hi Ryusuke,
> I have sent v2 patch and the root cause of the problem is in v2's commit
> message. I don't think this check is a stopgap, other filesystems, such
> like ext4, does this check too.
>
> nilfs_error will mark fs as readonly with default mount
> option(NILFS_MOUNT_ERRORS_RO), I am not sure if this is overkilled since
> this make the whole fs readonly(however, I add this in v2 patch).
> Perhaps we should isolate this error and only mark this file is bad ?
> For fs like ext4, we can mark one group to corrupted but nilfs seems not
> have this disk layout isolation.
> > Thanks,
> > Ryusuke Konishi
>
> --
> Regards

Hi Wang Jianjian,

Regarding this issue, as the analysis in your patch's commit message
indicates, the fundamental problem is that when the inode or directory
has sync write attributes set, nilfs_transaction_commit() calls
nilfs_construct_segment(), which receives an interruption and fails
with -ERESTARTSYS.

Consequently, the rename itself is retried even though it has already
modified metadata beyond the point of no return.

We need to consider proper countermeasures, such as prohibiting
interruptions during log write commits or making the transaction
properly rollback-capable.
Adding the i_nlink == 0 check is still merely a stopgap measure, and
since interruptions can occur during normal operations, we should not
trigger a filesystem error (or mark it read-only) because of them.

Handling this issue requires careful consideration.
Since we are late in the release cycle, I do not want to rush this
into the upstream tree right now.
Could we please split off the b-tree key sorting inconsistency check
patch to be processed first, and keep this rename issue under further
investigation?

Thanks,
Ryusuke Konishi