| Message ID | 20241122035159.441944-1-zhenghaoran@buaa.edu.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs: Fix data race in inode_set_ctime_to_ts | expand |
On Fri, Nov 22, 2024 at 11:51:59AM +0800, Hao-ran Zheng wrote:
> V2:
This doesn't apply because the functions you change have changed with
the vfs-6.14.mgtime merge. So please base your patch on current mainline
or vfs.fixes and resend. Thanks!
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@buaa.edu.cn> --- <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@buaa.edu.cn> > --- > 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
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@suse.cz> > Send time:Friday, 11/22/2024 19:22:28 > To: "Hao-ran Zheng" <zhenghaoran@buaa.edu.cn> > Cc: viro@zeniv.linux.org.uk, brauner@kernel.org, jack@suse.cz, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, baijiaju1990@gmail.com, 21371365@buaa.edu.cn > 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@buaa.edu.cn> > > --- > <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@buaa.edu.cn> > > --- > > 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@suse.com> > SUSE Labs, CR
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); } static inline long inode_get_ctime_nsec(const struct inode *inode) @@ -1674,8 +1674,8 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode) static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts) { - inode->i_ctime_sec = ts.tv_sec; - inode->i_ctime_nsec = ts.tv_nsec; + WRITE_ONCE(inode->i_ctime_sec, ts.tv_sec); + WRITE_ONCE(inode->i_ctime_nsec, ts.tv_nsec); return ts; }
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: ``` ============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() ``` To address this issue, it is recommended to add WRITE_ONCE when writing the `inode->i_ctime_sec` variable. -------------------------------------------------------------- Signed-off-by: Hao-ran Zheng <zhenghaoran@buaa.edu.cn> --- include/linux/fs.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)