linux-kernel - Re: [PATCH v2] fs: Fix data race in inode_set_ctime_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241122112228.6him45jdtibue26s@quack3>
Date: Fri, 22 Nov 2024 12:22:28 +0100
From: Jan Kara <jack@...e.cz>
To: Hao-ran Zheng <zhenghaoran@...a.edu.cn>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	baijiaju1990@...il.com, 21371365@...a.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@...a.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@...a.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@...e.com>
SUSE Labs, CR