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

在 2026/8/10 14:07, Ryusuke Konishi 写道:
> 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. will drop this.>
> Thanks,
> Ryusuke Konishi

-- 
Regards