Re: [PATCH v2] fat: fix parent link count when replacing a sibling directory

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> writes:

> Hungyu Lin <dennylin0707@xxxxxxxxx> writes:
>
>> vfat_rename() and do_msdos_rename() adjust parent link counts only when
>> updating the dotdot entry for a move between different parents. Replacing
>> an existing empty directory in the same parent also removes a
>> subdirectory, but leaves the parent's link count unchanged.
>>
>> For example, renaming one of two sibling directories over the other
>> leaves the parent with four links instead of three. stat and ls report
>> the stale count, and find -links 3 does not match the parent.
>>
>> Drop the parent's link count for this case after the source entry has
>> been removed successfully. Leave cross-parent renames, regular files
>> and the existing error paths unchanged.
>>
>> As in rmdir, check that the parent's link count is at least three before
>> decrementing it. Report a filesystem error if corrupted directory entries
>> have left the count too low, rather than reducing it below two.
>>
>> Acked-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Hungyu Lin <dennylin0707@xxxxxxxxx>
>> ---
>> Changes in v2:
>> - Check the parent link count before decrementing it and report low
>>   counts with fat_fs_error(), matching rmdir(), as noted by Sashiko.
>> - Retain Hirofumi's Acked-by from v1.
>>
>> v1: https://lore.kernel.org/r/20261028211626.26103-1-dennylin0707@xxxxxxxxx
>> Review: https://sashiko.dev/#/patchset/20261028211626.26103-1-dennylin0707@xxxxxxxxx
>>  fs/fat/namei_msdos.c | 8 ++++++++
>>  fs/fat/namei_vfat.c  | 8 ++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
>> index c7500c2859e0..97289c9d9261 100644
>> --- a/fs/fat/namei_msdos.c
>> +++ b/fs/fat/namei_msdos.c
>> @@ -565,6 +565,14 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
>>  		(void)sync_inode_metadata(old_dir, 1);
>>  
>>  	if (new_inode) {
>> +		if (is_dir && old_dir == new_dir) {
>> +			if (old_dir->i_nlink >= 3)
>> +				drop_nlink(old_dir);
>> +			else
>> +				fat_fs_error(old_dir->i_sb,
>> +					     "parent dir link count too low (%u)",
>> +					     old_dir->i_nlink);
>> +		}
>>  		drop_nlink(new_inode);
>>  		if (is_dir)
>>  			drop_nlink(new_inode);
>> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
>> index ff7652bf0410..c4d5628c05fa 100644
>> --- a/fs/fat/namei_vfat.c
>> +++ b/fs/fat/namei_vfat.c
>> @@ -1011,6 +1011,14 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
>>  	vfat_update_dir_metadata(old_dir, &ts);
>>  
>>  	if (new_inode) {
>> +		if (is_dir && old_dir == new_dir) {
>> +			if (old_dir->i_nlink >= 3)
>> +				drop_nlink(old_dir);
>> +			else
>> +				fat_fs_error(old_dir->i_sb,
>> +					     "parent dir link count too low (%u)",
>> +					     old_dir->i_nlink);
>> +		}
>>  		drop_nlink(new_inode);
>>  		if (is_dir)
>>  			drop_nlink(new_inode);
>
> Thanks. Hm, how about move those same code to fat module
>
> 	fat_dir_drop_nlink()
>         EXPORT_SYMBOL_GPL(fat_dir_drop_nlink)
>
> as such function for 3 places?

Ouch, it should be 4 places.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>