Thank you for your reply again. I am very sorry that the previous Patch V2 did not meet the submission requirements. I will carefully modify the email as required and submit Patch V3.
> -----Original Messages-----
> From: "Jan Kara" <jack@xxxxxxx>
> Send time:Friday, 11/22/2024 19:22:28
> To: "Hao-ran Zheng" <zhenghaoran@xxxxxxxxxxx>
> Cc: viro@xxxxxxxxxxxxxxxxxx, brauner@xxxxxxxxxx, jack@xxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, baijiaju1990@xxxxxxxxx, 21371365@xxxxxxxxxxx
> Subject: Re: [PATCH v2] fs: Fix data race in inode_set_ctime_to_ts
>
> On Fri 22-11-24 11:51:59, Hao-ran Zheng wrote:
> > V2:
> > Thanks for Honza's reply and suggestions. READ_ONCE should indeed
> > be added to the reading position. I added READ_ONCE to
> > `inode_get_ctime_sec()`. The new patch is as follows.
> > -----------------------------------------------------------------
> > V1:
> > A data race may occur when the function `inode_set_ctime_to_ts()` and
> > the function `inode_get_ctime_sec()` are executed concurrently. When
> > two threads call `aio_read` and `aio_write` respectively, they will
> > be distributed to the read and write functions of the corresponding
> > file system respectively. Taking the btrfs file system as an example,
> > the `btrfs_file_read_iter` and `btrfs_file_write_iter` functions are
> > finally called. These two functions created a data race when they
> > finally called `inode_get_ctime_sec()` and `inode_set_ctime_to_ns()`.
> > The specific call stack that appears during testing is as follows:
>
> Changelogs of the patch belong below the --- marker (so that they are not
> part of the final commit message). So this changelog should look like:
>
> A data race may occur when the function `inode_set_ctime_to_ts()` and
> the function `inode_get_ctime_sec()` are executed concurrently. When
> ....
>
> Signed-off-by: Hao-ran Zheng <zhenghaoran@xxxxxxxxxxx>
>
> ---
> <diffstat here>
>
> changes since v1:
> - ...
>
> <patch here>
>
> Please see 'The canonical patch format' chapter in
> Documentation/process/submitting-patches.rst for more details.
>
> > ```
>
> Also our changelogs are not in ReST or whatever other format. They are
> plain ASCII text. Hence quotes like above are pointless and mostly reducing
> readability.
>
> > ============DATA_RACE============
> > btrfs_delayed_update_inode+0x1f61/0x7ce0 [btrfs]
> > btrfs_update_inode+0x45e/0xbb0 [btrfs]
> > btrfs_dirty_inode+0x2b8/0x530 [btrfs]
> > btrfs_update_time+0x1ad/0x230 [btrfs]
> > touch_atime+0x211/0x440
> > filemap_read+0x90f/0xa20
> > btrfs_file_read_iter+0xeb/0x580 [btrfs]
> > aio_read+0x275/0x3a0
> > io_submit_one+0xd22/0x1ce0
> > __se_sys_io_submit+0xb3/0x250
> > do_syscall_64+0xc1/0x190
> > entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > ============OTHER_INFO============
> > btrfs_write_check+0xa15/0x1390 [btrfs]
> > btrfs_buffered_write+0x52f/0x29d0 [btrfs]
> > btrfs_do_write_iter+0x53d/0x1590 [btrfs]
> > btrfs_file_write_iter+0x41/0x60 [btrfs]
> > aio_write+0x41e/0x5f0
> > io_submit_one+0xd42/0x1ce0
> > __se_sys_io_submit+0xb3/0x250
> > do_syscall_64+0xc1/0x190
> > entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > ```
> >
> > The call chain after traceability is as follows:
> >
> > ```
> > Thread1:
> > btrfs_delayed_update_inode() ->
> > fill_stack_inode_item() ->
> > inode_get_ctime_sec()
> >
> > Thread2:
> > btrfs_write_check() ->
> > update_time_for_write() ->
> > inode_set_ctime_to_ts()
> > ```
>
> No need to repeat the stack traces again here. The output from KCSAN above
> is enough.
>
> > To address this issue, it is recommended to
> > add WRITE_ONCE when writing the `inode->i_ctime_sec` variable.
> > --------------------------------------------------------------
>
> Also this line of '-' is really unexpected. Please just leave empty line
> here.
>
> > Signed-off-by: Hao-ran Zheng <zhenghaoran@xxxxxxxxxxx>
> > ---
> > include/linux/fs.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3559446279c1..869ccfc9a787 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1655,7 +1655,7 @@ static inline struct timespec64 inode_set_mtime(struct inode *inode,
> >
> > static inline time64_t inode_get_ctime_sec(const struct inode *inode)
> > {
> > - return inode->i_ctime_sec;
> > + return READ_ONCE(inode->i_ctime_sec);
> > }
>
> Good. But please fix inode_get_ctime_nsec() as well.
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
