* [PATCH] xfs: fix data races in inode->i_*time
@ 2020-02-25 20:09 Qian Cai
2020-02-25 20:27 ` Marco Elver
2020-02-25 20:28 ` Darrick J. Wong
0 siblings, 2 replies; 4+ messages in thread
From: Qian Cai @ 2020-02-25 20:09 UTC (permalink / raw)
To: darrick.wong; +Cc: elver, linux-xfs, linux-kernel, Qian Cai
inode->i_*time could be accessed concurrently. The plain reads in
xfs_vn_getattr() is lockless which result in data races. To avoid bad
compiler optimizations like load tearing, adding pairs of
READ|WRITE_ONCE(). While at it, also take care of xfs_setattr_time()
which presumably could run concurrently with xfs_vn_getattr() as well.
The data races were reported by KCSAN,
write to 0xffff9275a1920ad8 of 16 bytes by task 47311 on cpu 46:
xfs_vn_update_time+0x1b0/0x400 [xfs]
xfs_vn_update_time at fs/xfs/xfs_iops.c:1122
update_time+0x57/0x80
file_update_time+0x143/0x1f0
__xfs_filemap_fault+0x1be/0x3d0 [xfs]
xfs_filemap_page_mkwrite+0x25/0x40 [xfs]
do_page_mkwrite+0xf7/0x250
do_fault+0x679/0x920
__handle_mm_fault+0xc9f/0xd40
handle_mm_fault+0xfc/0x2f0
do_page_fault+0x263/0x6f9
page_fault+0x34/0x40
4 locks held by doio/47311:
#0: ffff9275e7d70808 (&mm->mmap_sem#2){++++}, at: do_page_fault+0x143/0x6f9
#1: ffff9274864394d8 (sb_pagefaults){.+.+}, at: __xfs_filemap_fault+0x19b/0x3d0 [xfs]
#2: ffff9274864395b8 (sb_internal){.+.+}, at: xfs_trans_alloc+0x2af/0x3c0 [xfs]
#3: ffff9275a1920920 (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0x116/0x2c0 [xfs]
irq event stamp: 42649
hardirqs last enabled at (42649): [<ffffffffb22dcdb3>] _raw_spin_unlock_irqrestore+0x53/0x60
hardirqs last disabled at (42648): [<ffffffffb22dcad1>] _raw_spin_lock_irqsave+0x21/0x60
softirqs last enabled at (42306): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
softirqs last disabled at (42299): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
read to 0xffff9275a1920ad8 of 16 bytes by task 47312 on cpu 40:
xfs_vn_getattr+0x20c/0x6a0 [xfs]
xfs_vn_getattr at fs/xfs/xfs_iops.c:551
vfs_getattr_nosec+0x11a/0x170
vfs_statx_fd+0x54/0x90
__do_sys_newfstat+0x40/0x90
__x64_sys_newfstat+0x3a/0x50
do_syscall_64+0x91/0xb05
entry_SYSCALL_64_after_hwframe+0x49/0xbe
no locks held by doio/47312.
irq event stamp: 43883
hardirqs last enabled at (43883): [<ffffffffb1805119>] do_syscall_64+0x39/0xb05
hardirqs last disabled at (43882): [<ffffffffb1803ede>] trace_hardirqs_off_thunk+0x1a/0x1c
softirqs last enabled at (43844): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
softirqs last disabled at (43141): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
Signed-off-by: Qian Cai <cai@lca.pw>
---
fs/xfs/xfs_iops.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..2d5ca13ee9da 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -547,9 +547,9 @@
stat->uid = inode->i_uid;
stat->gid = inode->i_gid;
stat->ino = ip->i_ino;
- stat->atime = inode->i_atime;
- stat->mtime = inode->i_mtime;
- stat->ctime = inode->i_ctime;
+ stat->atime = READ_ONCE(inode->i_atime);
+ stat->mtime = READ_ONCE(inode->i_mtime);
+ stat->ctime = READ_ONCE(inode->i_ctime);
stat->blocks =
XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
@@ -614,11 +614,11 @@
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (iattr->ia_valid & ATTR_ATIME)
- inode->i_atime = iattr->ia_atime;
+ WRITE_ONCE(inode->i_atime, iattr->ia_atime);
if (iattr->ia_valid & ATTR_CTIME)
- inode->i_ctime = iattr->ia_ctime;
+ WRITE_ONCE(inode->i_ctime, iattr->ia_ctime);
if (iattr->ia_valid & ATTR_MTIME)
- inode->i_mtime = iattr->ia_mtime;
+ WRITE_ONCE(inode->i_mtime, iattr->ia_mtime);
}
static int
@@ -1117,11 +1117,11 @@
xfs_ilock(ip, XFS_ILOCK_EXCL);
if (flags & S_CTIME)
- inode->i_ctime = *now;
+ WRITE_ONCE(inode->i_ctime, *now);
if (flags & S_MTIME)
- inode->i_mtime = *now;
+ WRITE_ONCE(inode->i_mtime, *now);
if (flags & S_ATIME)
- inode->i_atime = *now;
+ WRITE_ONCE(inode->i_atime, *now);
xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
xfs_trans_log_inode(tp, ip, log_flags);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: fix data races in inode->i_*time
2020-02-25 20:09 [PATCH] xfs: fix data races in inode->i_*time Qian Cai
@ 2020-02-25 20:27 ` Marco Elver
2020-02-25 20:28 ` Darrick J. Wong
1 sibling, 0 replies; 4+ messages in thread
From: Marco Elver @ 2020-02-25 20:27 UTC (permalink / raw)
To: Qian Cai; +Cc: Darrick J. Wong, linux-xfs, LKML, kasan-dev
On Tue, 25 Feb 2020 at 21:09, Qian Cai <cai@lca.pw> wrote:
>
> inode->i_*time could be accessed concurrently. The plain reads in
> xfs_vn_getattr() is lockless which result in data races. To avoid bad
> compiler optimizations like load tearing, adding pairs of
> READ|WRITE_ONCE(). While at it, also take care of xfs_setattr_time()
> which presumably could run concurrently with xfs_vn_getattr() as well.
> The data races were reported by KCSAN,
>
> write to 0xffff9275a1920ad8 of 16 bytes by task 47311 on cpu 46:
> xfs_vn_update_time+0x1b0/0x400 [xfs]
> xfs_vn_update_time at fs/xfs/xfs_iops.c:1122
So this one is doing concurrent writes and reads of the whole struct,
which is 16 bytes. This will always be split into multiple
loads/stores. Is it intentional?
Sadly, this is pretty much guaranteed to tear, even with the
READ/WRITE_ONCE. The *ONCE will just make KCSAN not tell us about
this one, which is probably not what we want right now, unless we know
for sure the race is intentional.
Thanks,
-- Marco
> update_time+0x57/0x80
> file_update_time+0x143/0x1f0
> __xfs_filemap_fault+0x1be/0x3d0 [xfs]
> xfs_filemap_page_mkwrite+0x25/0x40 [xfs]
> do_page_mkwrite+0xf7/0x250
> do_fault+0x679/0x920
> __handle_mm_fault+0xc9f/0xd40
> handle_mm_fault+0xfc/0x2f0
> do_page_fault+0x263/0x6f9
> page_fault+0x34/0x40
>
> 4 locks held by doio/47311:
> #0: ffff9275e7d70808 (&mm->mmap_sem#2){++++}, at: do_page_fault+0x143/0x6f9
> #1: ffff9274864394d8 (sb_pagefaults){.+.+}, at: __xfs_filemap_fault+0x19b/0x3d0 [xfs]
> #2: ffff9274864395b8 (sb_internal){.+.+}, at: xfs_trans_alloc+0x2af/0x3c0 [xfs]
> #3: ffff9275a1920920 (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0x116/0x2c0 [xfs]
> irq event stamp: 42649
> hardirqs last enabled at (42649): [<ffffffffb22dcdb3>] _raw_spin_unlock_irqrestore+0x53/0x60
> hardirqs last disabled at (42648): [<ffffffffb22dcad1>] _raw_spin_lock_irqsave+0x21/0x60
> softirqs last enabled at (42306): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
> softirqs last disabled at (42299): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
>
> read to 0xffff9275a1920ad8 of 16 bytes by task 47312 on cpu 40:
> xfs_vn_getattr+0x20c/0x6a0 [xfs]
> xfs_vn_getattr at fs/xfs/xfs_iops.c:551
> vfs_getattr_nosec+0x11a/0x170
> vfs_statx_fd+0x54/0x90
> __do_sys_newfstat+0x40/0x90
> __x64_sys_newfstat+0x3a/0x50
> do_syscall_64+0x91/0xb05
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> no locks held by doio/47312.
> irq event stamp: 43883
> hardirqs last enabled at (43883): [<ffffffffb1805119>] do_syscall_64+0x39/0xb05
> hardirqs last disabled at (43882): [<ffffffffb1803ede>] trace_hardirqs_off_thunk+0x1a/0x1c
> softirqs last enabled at (43844): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
> softirqs last disabled at (43141): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> fs/xfs/xfs_iops.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..2d5ca13ee9da 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -547,9 +547,9 @@
> stat->uid = inode->i_uid;
> stat->gid = inode->i_gid;
> stat->ino = ip->i_ino;
> - stat->atime = inode->i_atime;
> - stat->mtime = inode->i_mtime;
> - stat->ctime = inode->i_ctime;
> + stat->atime = READ_ONCE(inode->i_atime);
> + stat->mtime = READ_ONCE(inode->i_mtime);
> + stat->ctime = READ_ONCE(inode->i_ctime);
> stat->blocks =
> XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>
> @@ -614,11 +614,11 @@
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (iattr->ia_valid & ATTR_ATIME)
> - inode->i_atime = iattr->ia_atime;
> + WRITE_ONCE(inode->i_atime, iattr->ia_atime);
> if (iattr->ia_valid & ATTR_CTIME)
> - inode->i_ctime = iattr->ia_ctime;
> + WRITE_ONCE(inode->i_ctime, iattr->ia_ctime);
> if (iattr->ia_valid & ATTR_MTIME)
> - inode->i_mtime = iattr->ia_mtime;
> + WRITE_ONCE(inode->i_mtime, iattr->ia_mtime);
> }
>
> static int
> @@ -1117,11 +1117,11 @@
>
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> if (flags & S_CTIME)
> - inode->i_ctime = *now;
> + WRITE_ONCE(inode->i_ctime, *now);
> if (flags & S_MTIME)
> - inode->i_mtime = *now;
> + WRITE_ONCE(inode->i_mtime, *now);
> if (flags & S_ATIME)
> - inode->i_atime = *now;
> + WRITE_ONCE(inode->i_atime, *now);
>
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> xfs_trans_log_inode(tp, ip, log_flags);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: fix data races in inode->i_*time
2020-02-25 20:09 [PATCH] xfs: fix data races in inode->i_*time Qian Cai
2020-02-25 20:27 ` Marco Elver
@ 2020-02-25 20:28 ` Darrick J. Wong
2020-02-25 20:46 ` Christoph Hellwig
1 sibling, 1 reply; 4+ messages in thread
From: Darrick J. Wong @ 2020-02-25 20:28 UTC (permalink / raw)
To: Qian Cai; +Cc: elver, linux-xfs, linux-kernel
On Tue, Feb 25, 2020 at 03:09:45PM -0500, Qian Cai wrote:
> inode->i_*time could be accessed concurrently. The plain reads in
> xfs_vn_getattr() is lockless which result in data races. To avoid bad
> compiler optimizations like load tearing, adding pairs of
> READ|WRITE_ONCE(). While at it, also take care of xfs_setattr_time()
> which presumably could run concurrently with xfs_vn_getattr() as well.
> The data races were reported by KCSAN,
>
> write to 0xffff9275a1920ad8 of 16 bytes by task 47311 on cpu 46:
> xfs_vn_update_time+0x1b0/0x400 [xfs]
> xfs_vn_update_time at fs/xfs/xfs_iops.c:1122
> update_time+0x57/0x80
> file_update_time+0x143/0x1f0
> __xfs_filemap_fault+0x1be/0x3d0 [xfs]
> xfs_filemap_page_mkwrite+0x25/0x40 [xfs]
> do_page_mkwrite+0xf7/0x250
> do_fault+0x679/0x920
> __handle_mm_fault+0xc9f/0xd40
> handle_mm_fault+0xfc/0x2f0
> do_page_fault+0x263/0x6f9
> page_fault+0x34/0x40
>
> 4 locks held by doio/47311:
> #0: ffff9275e7d70808 (&mm->mmap_sem#2){++++}, at: do_page_fault+0x143/0x6f9
> #1: ffff9274864394d8 (sb_pagefaults){.+.+}, at: __xfs_filemap_fault+0x19b/0x3d0 [xfs]
> #2: ffff9274864395b8 (sb_internal){.+.+}, at: xfs_trans_alloc+0x2af/0x3c0 [xfs]
> #3: ffff9275a1920920 (&xfs_nondir_ilock_class){++++}, at: xfs_ilock+0x116/0x2c0 [xfs]
> irq event stamp: 42649
> hardirqs last enabled at (42649): [<ffffffffb22dcdb3>] _raw_spin_unlock_irqrestore+0x53/0x60
> hardirqs last disabled at (42648): [<ffffffffb22dcad1>] _raw_spin_lock_irqsave+0x21/0x60
> softirqs last enabled at (42306): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
> softirqs last disabled at (42299): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
>
> read to 0xffff9275a1920ad8 of 16 bytes by task 47312 on cpu 40:
> xfs_vn_getattr+0x20c/0x6a0 [xfs]
> xfs_vn_getattr at fs/xfs/xfs_iops.c:551
> vfs_getattr_nosec+0x11a/0x170
> vfs_statx_fd+0x54/0x90
> __do_sys_newfstat+0x40/0x90
> __x64_sys_newfstat+0x3a/0x50
> do_syscall_64+0x91/0xb05
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> no locks held by doio/47312.
> irq event stamp: 43883
> hardirqs last enabled at (43883): [<ffffffffb1805119>] do_syscall_64+0x39/0xb05
> hardirqs last disabled at (43882): [<ffffffffb1803ede>] trace_hardirqs_off_thunk+0x1a/0x1c
> softirqs last enabled at (43844): [<ffffffffb260034c>] __do_softirq+0x34c/0x57c
> softirqs last disabled at (43141): [<ffffffffb18c6762>] irq_exit+0xa2/0xc0
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
> fs/xfs/xfs_iops.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..2d5ca13ee9da 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -547,9 +547,9 @@
> stat->uid = inode->i_uid;
> stat->gid = inode->i_gid;
> stat->ino = ip->i_ino;
> - stat->atime = inode->i_atime;
> - stat->mtime = inode->i_mtime;
> - stat->ctime = inode->i_ctime;
> + stat->atime = READ_ONCE(inode->i_atime);
> + stat->mtime = READ_ONCE(inode->i_mtime);
> + stat->ctime = READ_ONCE(inode->i_ctime);
Seeing as one is supposed to take ILOCK_SHARED before reading inode core
information, why don't we do that here? Is there some huge performance
benefit to be realized from READ_ONCE vs. waiting for the lock that
protects all the writes from each other?
--D
> stat->blocks =
> XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>
> @@ -614,11 +614,11 @@
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (iattr->ia_valid & ATTR_ATIME)
> - inode->i_atime = iattr->ia_atime;
> + WRITE_ONCE(inode->i_atime, iattr->ia_atime);
> if (iattr->ia_valid & ATTR_CTIME)
> - inode->i_ctime = iattr->ia_ctime;
> + WRITE_ONCE(inode->i_ctime, iattr->ia_ctime);
> if (iattr->ia_valid & ATTR_MTIME)
> - inode->i_mtime = iattr->ia_mtime;
> + WRITE_ONCE(inode->i_mtime, iattr->ia_mtime);
> }
>
> static int
> @@ -1117,11 +1117,11 @@
>
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> if (flags & S_CTIME)
> - inode->i_ctime = *now;
> + WRITE_ONCE(inode->i_ctime, *now);
> if (flags & S_MTIME)
> - inode->i_mtime = *now;
> + WRITE_ONCE(inode->i_mtime, *now);
> if (flags & S_ATIME)
> - inode->i_atime = *now;
> + WRITE_ONCE(inode->i_atime, *now);
>
> xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> xfs_trans_log_inode(tp, ip, log_flags);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: fix data races in inode->i_*time
2020-02-25 20:28 ` Darrick J. Wong
@ 2020-02-25 20:46 ` Christoph Hellwig
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2020-02-25 20:46 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Qian Cai, elver, linux-xfs, linux-kernel
On Tue, Feb 25, 2020 at 12:28:29PM -0800, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 81f2f93caec0..2d5ca13ee9da 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -547,9 +547,9 @@
> > stat->uid = inode->i_uid;
> > stat->gid = inode->i_gid;
> > stat->ino = ip->i_ino;
> > - stat->atime = inode->i_atime;
> > - stat->mtime = inode->i_mtime;
> > - stat->ctime = inode->i_ctime;
> > + stat->atime = READ_ONCE(inode->i_atime);
> > + stat->mtime = READ_ONCE(inode->i_mtime);
> > + stat->ctime = READ_ONCE(inode->i_ctime);
>
> Seeing as one is supposed to take ILOCK_SHARED before reading inode core
> information, why don't we do that here? Is there some huge performance
> benefit to be realized from READ_ONCE vs. waiting for the lock that
> protects all the writes from each other?
Yes, I don't see how READ_ONCE works on a structure.
I think you should look into fixing this race in generic_fillattr
first, and we then piggy back on that fix in XFS once it has all been
sorted out.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-25 20:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-25 20:09 [PATCH] xfs: fix data races in inode->i_*time Qian Cai
2020-02-25 20:27 ` Marco Elver
2020-02-25 20:28 ` Darrick J. Wong
2020-02-25 20:46 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox