The i_pos field remains uninitialized when fat_fs_error_ratelimit() reports
error, e.g.,

	[ 1642.703550] FAT-fs (loop0): error, fat_get_cluster: invalid
	cluster chain (i_pos 0)

Since i_pos is assigned in fat_attach after fat_fill_inode, the error
message lacks useful debug info.

Path:
vfat_lookup
	fat_build_inode
		fat_fill_inode
			fat_calc_dir_size
          			fat_get_cluster /* report error */
      	fat_attach  /* i_pos assigned here */

Reproduction steps:
dd if=/dev/zero of=/tmp/fatfile bs=1M count=1024 && \
mkfs.vfat -I /tmp/fatfile && \
fsck.vfat -bv /tmp/fatfile && \
mount -t vfat /tmp/fatfile /mnt/vfat && \
mkdir /mnt/vfat/dir1 && \
for i in `seq 1 500`;do touch /mnt/vfat/dir1/fatregfiletest.${i};done && \
dd if=/dev/zero of=/dev/loop0 bs=1 count=1 seek=$((16384+0x1C)) && \
sync && echo 3 > /proc/sys/vm/drop_caches && \
ls /mnt/vfat/dir1/

Signed-off-by: zhoumin <teczm@foxmail.com>
---
 fs/fat/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3852bb66358c..c71f08b20617 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -405,7 +405,6 @@ void fat_attach(struct inode *inode, loff_t i_pos)
 					  + fat_hash(i_pos);
 
 		spin_lock(&sbi->inode_hash_lock);
-		MSDOS_I(inode)->i_pos = i_pos;
 		hlist_add_head(&MSDOS_I(inode)->i_fat_hash, head);
 		spin_unlock(&sbi->inode_hash_lock);
 	}
@@ -429,7 +428,6 @@ void fat_detach(struct inode *inode)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
 	spin_lock(&sbi->inode_hash_lock);
-	MSDOS_I(inode)->i_pos = 0;
 	hlist_del_init(&MSDOS_I(inode)->i_fat_hash);
 	spin_unlock(&sbi->inode_hash_lock);
 
@@ -513,7 +511,6 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
 	struct timespec64 mtime;
 	int error;
 
-	MSDOS_I(inode)->i_pos = 0;
 	inode->i_uid = sbi->options.fs_uid;
 	inode->i_gid = sbi->options.fs_gid;
 	inode_inc_iversion(inode);
@@ -604,6 +601,7 @@ struct inode *fat_build_inode(struct super_block *sb,
 		goto out;
 	}
 	inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
+	MSDOS_I(inode)->i_pos = i_pos;
 	inode_set_iversion(inode, 1);
 	err = fat_fill_inode(inode, de);
 	if (err) {
-- 
2.43.0

zhoumin <teczm@foxmail.com> writes:

> The i_pos field remains uninitialized when fat_fs_error_ratelimit() reports
> error, e.g.,
>
> 	[ 1642.703550] FAT-fs (loop0): error, fat_get_cluster: invalid
> 	cluster chain (i_pos 0)
>
> Since i_pos is assigned in fat_attach after fat_fill_inode, the error
> message lacks useful debug info.
>
> Path:
> vfat_lookup
> 	fat_build_inode
> 		fat_fill_inode
> 			fat_calc_dir_size
>           			fat_get_cluster /* report error */
>       	fat_attach  /* i_pos assigned here */

No. It is initialized as 0, and it must be unavailable outside
between fat_attach and fat_detach.

IOW, this is introducing the race.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Hi OGAWA-san,

Thank you for your review and feedback.

>> The i_pos field remains uninitialized when fat_fs_error_ratelimit() reports
>> error, e.g.,
>>
>> 	[ 1642.703550] FAT-fs (loop0): error, fat_get_cluster: invalid
>> 	cluster chain (i_pos 0)
>>
>> Since i_pos is assigned in fat_attach after fat_fill_inode, the error
>> message lacks useful debug info.
>>
>> Path:
>> vfat_lookup
>> 	fat_build_inode
>> 		fat_fill_inode
>> 			fat_calc_dir_size
>>           			fat_get_cluster /* report error */
>>       	fat_attach  /* i_pos assigned here */

> No. It is initialized as 0, and it must be unavailable outside
> between fat_attach and fat_detach.

I see that it was initialized to 0. What I meant is that when
fat_fs_error_ratelimit uses i_pos for debugging output, it doesn't get the
correct value.

> IOW, this is introducing the race.
I'm not quite clear about the race condition you mentioned here. Could you
give an example to explain it?

Actually, the modification I initially considered was as follows. I'm not
sure if this approach carries any risks. If it's acceptable, I'm ready to
submit a new version.

Thank you.


diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 3852bb66358c..837b1ad361ca 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -605,13 +605,14 @@ struct inode *fat_build_inode(struct super_block *sb,
        }
        inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
        inode_set_iversion(inode, 1);
+       fat_attach(inode, i_pos);
        err = fat_fill_inode(inode, de);
        if (err) {
                iput(inode);
+               fat_detach(inode);
                inode = ERR_PTR(err);
                goto out;
-       }
-       fat_attach(inode, i_pos);
+       }
        insert_inode_hash(inode);
 out:
        fat_unlock_build_inode(MSDOS_SB(sb));

Best regards,
zhoumin

zhoumin <teczm@foxmail.com> writes:

>>> The i_pos field remains uninitialized when fat_fs_error_ratelimit() reports
>>> error, e.g.,
>>>
>>> 	[ 1642.703550] FAT-fs (loop0): error, fat_get_cluster: invalid
>>> 	cluster chain (i_pos 0)
>>>
>>> Since i_pos is assigned in fat_attach after fat_fill_inode, the error
>>> message lacks useful debug info.
>>>
>>> Path:
>>> vfat_lookup
>>> 	fat_build_inode
>>> 		fat_fill_inode
>>> 			fat_calc_dir_size
>>>           			fat_get_cluster /* report error */
>>>       	fat_attach  /* i_pos assigned here */
> 
>> No. It is initialized as 0, and it must be unavailable outside
>> between fat_attach and fat_detach.
> 
> I see that it was initialized to 0. What I meant is that when
> fat_fs_error_ratelimit uses i_pos for debugging output, it doesn't get the
> correct value.

Not big issue. IOW, 0 is still a valid state.

>> IOW, this is introducing the race.
> I'm not quite clear about the race condition you mentioned here. Could you
> give an example to explain it?
> 
> Actually, the modification I initially considered was as follows. I'm not
> sure if this approach carries any risks. If it's acceptable, I'm ready to
> submit a new version.

When you try to change logic, you should read and understand the current
logic before.

For example, fat_iget() and __fat_write_inode() have to grab only valid
and live inode. And the orphaned inode must not be written back to disk,
because same entry can be reused already.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Hi OGAWA-san,

Thank you for the explanation.

I apologize for the noise and my lack of understanding. I will look into
the code you pointed out  and explore if there's a better approach to
address the original issue.

Best regards,
zhoumin