* [PATCH] ext4: fix race condition between buffer write and page_mkwrite
@ 2023-05-29 8:01 Baokun Li
2023-05-29 14:44 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Baokun Li @ 2023-05-29 8:01 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, jun.nie,
ebiggers, yi.zhang, yangerkun, yukuai3, libaokun1,
syzbot+a158d886ca08a3fecca4, stable
Syzbot reported a BUG_ON:
==================================================================
EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
------------[ cut here ]------------
kernel BUG at fs/ext4/ext4_jbd2.c:53!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
[...]
Call Trace:
ext4_write_inline_data_end+0xa39/0xdf0
ext4_da_write_end+0x1e2/0x950
generic_perform_write+0x401/0x5f0
ext4_buffered_write_iter+0x35f/0x640
ext4_file_write_iter+0x198/0x1cd0
vfs_write+0x8b5/0xef0
[...]
==================================================================
The above BUG_ON is triggered by the following race:
cpu1 cpu2
________________________|________________________
ksys_write
vfs_write
new_sync_write
ext4_file_write_iter
ext4_buffered_write_iter
generic_perform_write
ext4_da_write_begin
do_fault
do_page_mkwrite
ext4_page_mkwrite
ext4_convert_inline_data
ext4_convert_inline_data_nolock
ext4_destroy_inline_data_nolock
//clear EXT4_STATE_MAY_INLINE_DATA
ext4_map_blocks --> return error
ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
ext4_block_write_begin
ext4_restore_inline_data
// set EXT4_STATE_MAY_INLINE_DATA
ext4_da_write_end
ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
ext4_write_inline_data_end
handle=NULL
ext4_journal_stop(handle)
__ext4_journal_stop
ext4_put_nojournal(handle)
ref_cnt = (unsigned long)handle
BUG_ON(ref_cnt == 0) ---> BUG_ON
The root cause of this problem is that the ext4_convert_inline_data() in
ext4_page_mkwrite() does not grab i_rwsem, so it may race with
ext4_buffered_write_iter() and cause the write_begin() and write_end()
functions to be inconsistent and trigger BUG_ON.
To solve the above issue, we cannot add inode_lock directly to
ext4_page_mkwrite(), because this function is a hot path and frequent calls
to inode_lock will cause performance degradation for multi-threaded reads
and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
and only when inline_data is enabled and mmap a file in shared write mode,
we hold the lock to convert, which can reduce the impact on performance.
Reported-by: Jun Nie <jun.nie@linaro.org>
Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
CC: stable@vger.kernel.org # 4.12+
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
fs/ext4/file.c | 24 +++++++++++++++++++++++-
fs/ext4/inode.c | 4 ----
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d101b3b0c7da..7a04376c33f2 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -795,7 +795,8 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
{
struct inode *inode = file->f_mapping->host;
- struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+ struct super_block *sb = inode->i_sb;
+ struct ext4_sb_info *sbi = EXT4_SB(sb);
struct dax_device *dax_dev = sbi->s_daxdev;
if (unlikely(ext4_forced_shutdown(sbi)))
@@ -808,6 +809,27 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
if (!daxdev_mapping_supported(vma, dax_dev))
return -EOPNOTSUPP;
+ /*
+ * Writing via mmap has no logic to handle inline data, so we
+ * need to call ext4_convert_inline_data() to convert the inode
+ * to normal format before doing so, otherwise a BUG_ON will be
+ * triggered in ext4_writepages() due to the
+ * EXT4_STATE_MAY_INLINE_DATA flag. Moreover, we need to grab
+ * i_rwsem during conversion, since clearing and setting the
+ * inline data flag may race with ext4_buffered_write_iter()
+ * to trigger a BUG_ON.
+ */
+ if (ext4_has_feature_inline_data(sb) &&
+ vma->vm_flags & VM_SHARED && vma->vm_flags & VM_WRITE) {
+ int err;
+
+ inode_lock(inode);
+ err = ext4_convert_inline_data(inode);
+ inode_unlock(inode);
+ if (err)
+ return err;
+ }
+
file_accessed(file);
if (IS_DAX(file_inode(file))) {
vma->vm_ops = &ext4_dax_vm_ops;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ce5f21b6c2b3..31844c4ec9fe 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6043,10 +6043,6 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
filemap_invalidate_lock_shared(mapping);
- err = ext4_convert_inline_data(inode);
- if (err)
- goto out_ret;
-
/*
* On data journalling we skip straight to the transaction handle:
* there's no delalloc; page truncated will be checked later; the
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-29 8:01 [PATCH] ext4: fix race condition between buffer write and page_mkwrite Baokun Li
@ 2023-05-29 14:44 ` Jan Kara
2023-05-30 2:00 ` Baokun Li
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2023-05-29 14:44 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable
On Mon 29-05-23 16:01:48, Baokun Li wrote:
> Syzbot reported a BUG_ON:
> ==================================================================
> EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
> EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
> bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
> ------------[ cut here ]------------
> kernel BUG at fs/ext4/ext4_jbd2.c:53!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
> RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
> [...]
> Call Trace:
> ext4_write_inline_data_end+0xa39/0xdf0
> ext4_da_write_end+0x1e2/0x950
> generic_perform_write+0x401/0x5f0
> ext4_buffered_write_iter+0x35f/0x640
> ext4_file_write_iter+0x198/0x1cd0
> vfs_write+0x8b5/0xef0
> [...]
> ==================================================================
>
> The above BUG_ON is triggered by the following race:
>
> cpu1 cpu2
> ________________________|________________________
> ksys_write
> vfs_write
> new_sync_write
> ext4_file_write_iter
> ext4_buffered_write_iter
> generic_perform_write
> ext4_da_write_begin
> do_fault
> do_page_mkwrite
> ext4_page_mkwrite
> ext4_convert_inline_data
> ext4_convert_inline_data_nolock
> ext4_destroy_inline_data_nolock
> //clear EXT4_STATE_MAY_INLINE_DATA
> ext4_map_blocks --> return error
> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> ext4_block_write_begin
> ext4_restore_inline_data
> // set EXT4_STATE_MAY_INLINE_DATA
> ext4_da_write_end
> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> ext4_write_inline_data_end
> handle=NULL
> ext4_journal_stop(handle)
> __ext4_journal_stop
> ext4_put_nojournal(handle)
> ref_cnt = (unsigned long)handle
> BUG_ON(ref_cnt == 0) ---> BUG_ON
>
> The root cause of this problem is that the ext4_convert_inline_data() in
> ext4_page_mkwrite() does not grab i_rwsem, so it may race with
> ext4_buffered_write_iter() and cause the write_begin() and write_end()
> functions to be inconsistent and trigger BUG_ON.
>
> To solve the above issue, we cannot add inode_lock directly to
> ext4_page_mkwrite(), because this function is a hot path and frequent calls
> to inode_lock will cause performance degradation for multi-threaded reads
> and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
> and only when inline_data is enabled and mmap a file in shared write mode,
> we hold the lock to convert, which can reduce the impact on performance.
>
> Reported-by: Jun Nie <jun.nie@linaro.org>
> Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
> Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
> Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
> CC: stable@vger.kernel.org # 4.12+
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
not so much about performance as about lock ordering. In
ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
acquire i_rwsem because it ranks about it.
> ---
> fs/ext4/file.c | 24 +++++++++++++++++++++++-
> fs/ext4/inode.c | 4 ----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index d101b3b0c7da..7a04376c33f2 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -795,7 +795,8 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
> static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
> {
> struct inode *inode = file->f_mapping->host;
> - struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + struct super_block *sb = inode->i_sb;
> + struct ext4_sb_info *sbi = EXT4_SB(sb);
> struct dax_device *dax_dev = sbi->s_daxdev;
>
> if (unlikely(ext4_forced_shutdown(sbi)))
> @@ -808,6 +809,27 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
> if (!daxdev_mapping_supported(vma, dax_dev))
> return -EOPNOTSUPP;
>
> + /*
> + * Writing via mmap has no logic to handle inline data, so we
> + * need to call ext4_convert_inline_data() to convert the inode
> + * to normal format before doing so, otherwise a BUG_ON will be
> + * triggered in ext4_writepages() due to the
> + * EXT4_STATE_MAY_INLINE_DATA flag. Moreover, we need to grab
> + * i_rwsem during conversion, since clearing and setting the
> + * inline data flag may race with ext4_buffered_write_iter()
> + * to trigger a BUG_ON.
> + */
> + if (ext4_has_feature_inline_data(sb) &&
> + vma->vm_flags & VM_SHARED && vma->vm_flags & VM_WRITE) {
Sadly this does not work because we can mmap(2) the file read-only and then
use mprotect(2) to make file writeable. But we can test for VM_MAYWRITE
which gets set when mapping can be made writeable (basically anytime when
the file descriptor itself is writeable).
Otherwise the patch looks good.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-29 14:44 ` Jan Kara
@ 2023-05-30 2:00 ` Baokun Li
2023-05-30 7:42 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Baokun Li @ 2023-05-30 2:00 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable
On 2023/5/29 22:44, Jan Kara wrote:
> On Mon 29-05-23 16:01:48, Baokun Li wrote:
>> Syzbot reported a BUG_ON:
>> ==================================================================
>> EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
>> EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
>> bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
>> ------------[ cut here ]------------
>> kernel BUG at fs/ext4/ext4_jbd2.c:53!
>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>> CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
>> RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
>> [...]
>> Call Trace:
>> ext4_write_inline_data_end+0xa39/0xdf0
>> ext4_da_write_end+0x1e2/0x950
>> generic_perform_write+0x401/0x5f0
>> ext4_buffered_write_iter+0x35f/0x640
>> ext4_file_write_iter+0x198/0x1cd0
>> vfs_write+0x8b5/0xef0
>> [...]
>> ==================================================================
>>
>> The above BUG_ON is triggered by the following race:
>>
>> cpu1 cpu2
>> ________________________|________________________
>> ksys_write
>> vfs_write
>> new_sync_write
>> ext4_file_write_iter
>> ext4_buffered_write_iter
>> generic_perform_write
>> ext4_da_write_begin
>> do_fault
>> do_page_mkwrite
>> ext4_page_mkwrite
>> ext4_convert_inline_data
>> ext4_convert_inline_data_nolock
>> ext4_destroy_inline_data_nolock
>> //clear EXT4_STATE_MAY_INLINE_DATA
>> ext4_map_blocks --> return error
>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>> ext4_block_write_begin
>> ext4_restore_inline_data
>> // set EXT4_STATE_MAY_INLINE_DATA
>> ext4_da_write_end
>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>> ext4_write_inline_data_end
>> handle=NULL
>> ext4_journal_stop(handle)
>> __ext4_journal_stop
>> ext4_put_nojournal(handle)
>> ref_cnt = (unsigned long)handle
>> BUG_ON(ref_cnt == 0) ---> BUG_ON
>>
>> The root cause of this problem is that the ext4_convert_inline_data() in
>> ext4_page_mkwrite() does not grab i_rwsem, so it may race with
>> ext4_buffered_write_iter() and cause the write_begin() and write_end()
>> functions to be inconsistent and trigger BUG_ON.
>>
>> To solve the above issue, we cannot add inode_lock directly to
>> ext4_page_mkwrite(), because this function is a hot path and frequent calls
>> to inode_lock will cause performance degradation for multi-threaded reads
>> and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
>> and only when inline_data is enabled and mmap a file in shared write mode,
>> we hold the lock to convert, which can reduce the impact on performance.
>>
>> Reported-by: Jun Nie <jun.nie@linaro.org>
>> Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
>> Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
>> Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
>> CC: stable@vger.kernel.org # 4.12+
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
> not so much about performance as about lock ordering. In
> ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
> acquire i_rwsem because it ranks about it.
Thank you for your review!
I'm sorry I didn't make myself clear here.
Yes, we can't get i_rwsem after holding mmap_sem at any time, otherwise
ABBA deadlock may occur. The "add inode_lock directly" in my patch
description actually looks like this:
```
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b98d2d58b900..c9318dc2a613 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6025,12 +6025,14 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
sb_start_pagefault(inode->i_sb);
file_update_time(vma->vm_file);
- filemap_invalidate_lock_shared(mapping);
-
+ inode_lock(inode);
err = ext4_convert_inline_data(inode);
+ inode_unlock(inode);
if (err)
goto out_ret;
+ filemap_invalidate_lock_shared(mapping);
+
/*
* On data journalling we skip straight to the transaction handle:
* there's no delalloc; page truncated will be checked later; the
```
Originally, ext4_page_mkwrite could be freely concurrent with write,
but after adding inode_lock as in the above scenario, the two would
compete for inode_lock, thus having a performance impact on the
scenario where ext4_page_mkwrite and write are called at the same
time. The ext4_file_map will definitely be called before calling
ext4_page_mkwrite, and ext4_file_map will be called much less often,
so the logic for locking and performing convert is moved here to
reduce the performance impact.
>> ---
>> fs/ext4/file.c | 24 +++++++++++++++++++++++-
>> fs/ext4/inode.c | 4 ----
>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index d101b3b0c7da..7a04376c33f2 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -795,7 +795,8 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
>> static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>> {
>> struct inode *inode = file->f_mapping->host;
>> - struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>> + struct super_block *sb = inode->i_sb;
>> + struct ext4_sb_info *sbi = EXT4_SB(sb);
>> struct dax_device *dax_dev = sbi->s_daxdev;
>>
>> if (unlikely(ext4_forced_shutdown(sbi)))
>> @@ -808,6 +809,27 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>> if (!daxdev_mapping_supported(vma, dax_dev))
>> return -EOPNOTSUPP;
>>
>> + /*
>> + * Writing via mmap has no logic to handle inline data, so we
>> + * need to call ext4_convert_inline_data() to convert the inode
>> + * to normal format before doing so, otherwise a BUG_ON will be
>> + * triggered in ext4_writepages() due to the
>> + * EXT4_STATE_MAY_INLINE_DATA flag. Moreover, we need to grab
>> + * i_rwsem during conversion, since clearing and setting the
>> + * inline data flag may race with ext4_buffered_write_iter()
>> + * to trigger a BUG_ON.
>> + */
>> + if (ext4_has_feature_inline_data(sb) &&
>> + vma->vm_flags & VM_SHARED && vma->vm_flags & VM_WRITE) {
> Sadly this does not work because we can mmap(2) the file read-only and then
> use mprotect(2) to make file writeable.
I'm very sorry I didn't take this situation into account.
Thank you for pointing out the problem!
> But we can test for VM_MAYWRITE
> which gets set when mapping can be made writeable (basically anytime when
> the file descriptor itself is writeable).
>
> Otherwise the patch looks good.
>
> Honza
Indeed!
I will send a patch V2 with the changes suggested by you.
Thanks again!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-30 2:00 ` Baokun Li
@ 2023-05-30 7:42 ` Jan Kara
2023-05-30 9:39 ` Baokun Li
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2023-05-30 7:42 UTC (permalink / raw)
To: Baokun Li
Cc: Jan Kara, linux-ext4, tytso, adilger.kernel, ritesh.list,
linux-kernel, jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable
On Tue 30-05-23 10:00:44, Baokun Li wrote:
> On 2023/5/29 22:44, Jan Kara wrote:
> > On Mon 29-05-23 16:01:48, Baokun Li wrote:
> > > Syzbot reported a BUG_ON:
> > > ==================================================================
> > > EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
> > > EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
> > > bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
> > > ------------[ cut here ]------------
> > > kernel BUG at fs/ext4/ext4_jbd2.c:53!
> > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
> > > RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
> > > [...]
> > > Call Trace:
> > > ext4_write_inline_data_end+0xa39/0xdf0
> > > ext4_da_write_end+0x1e2/0x950
> > > generic_perform_write+0x401/0x5f0
> > > ext4_buffered_write_iter+0x35f/0x640
> > > ext4_file_write_iter+0x198/0x1cd0
> > > vfs_write+0x8b5/0xef0
> > > [...]
> > > ==================================================================
> > >
> > > The above BUG_ON is triggered by the following race:
> > >
> > > cpu1 cpu2
> > > ________________________|________________________
> > > ksys_write
> > > vfs_write
> > > new_sync_write
> > > ext4_file_write_iter
> > > ext4_buffered_write_iter
> > > generic_perform_write
> > > ext4_da_write_begin
> > > do_fault
> > > do_page_mkwrite
> > > ext4_page_mkwrite
> > > ext4_convert_inline_data
> > > ext4_convert_inline_data_nolock
> > > ext4_destroy_inline_data_nolock
> > > //clear EXT4_STATE_MAY_INLINE_DATA
> > > ext4_map_blocks --> return error
> > > ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> > > ext4_block_write_begin
> > > ext4_restore_inline_data
> > > // set EXT4_STATE_MAY_INLINE_DATA
> > > ext4_da_write_end
> > > ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> > > ext4_write_inline_data_end
> > > handle=NULL
> > > ext4_journal_stop(handle)
> > > __ext4_journal_stop
> > > ext4_put_nojournal(handle)
> > > ref_cnt = (unsigned long)handle
> > > BUG_ON(ref_cnt == 0) ---> BUG_ON
> > >
> > > The root cause of this problem is that the ext4_convert_inline_data() in
> > > ext4_page_mkwrite() does not grab i_rwsem, so it may race with
> > > ext4_buffered_write_iter() and cause the write_begin() and write_end()
> > > functions to be inconsistent and trigger BUG_ON.
> > >
> > > To solve the above issue, we cannot add inode_lock directly to
> > > ext4_page_mkwrite(), because this function is a hot path and frequent calls
> > > to inode_lock will cause performance degradation for multi-threaded reads
> > > and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
> > > and only when inline_data is enabled and mmap a file in shared write mode,
> > > we hold the lock to convert, which can reduce the impact on performance.
> > >
> > > Reported-by: Jun Nie <jun.nie@linaro.org>
> > > Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
> > > Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
> > > Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
> > > CC: stable@vger.kernel.org # 4.12+
> > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
> > not so much about performance as about lock ordering. In
> > ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
> > acquire i_rwsem because it ranks about it.
>
> Thank you for your review!
>
> I'm sorry I didn't make myself clear here.
>
> Yes, we can't get i_rwsem after holding mmap_sem at any time, otherwise
> ABBA deadlock may occur. The "add inode_lock directly" in my patch
> description actually looks like this:
> ```
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b98d2d58b900..c9318dc2a613 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6025,12 +6025,14 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
> sb_start_pagefault(inode->i_sb);
> file_update_time(vma->vm_file);
>
> - filemap_invalidate_lock_shared(mapping);
> -
> + inode_lock(inode);
> err = ext4_convert_inline_data(inode);
> + inode_unlock(inode);
> if (err)
> goto out_ret;
>
> + filemap_invalidate_lock_shared(mapping);
> +
> /*
> * On data journalling we skip straight to the transaction handle:
> * there's no delalloc; page truncated will be checked later; the
> ```
Yes, but even this could deadlock. The ABBA deadlock I'm speaking about
would not be created with mapping->invalidate_lock but rather with
task->mm->mmap_lock which is acquired at the beginning of page fault in the
arch code. And when you do write(2), you hold inode_lock() and then you
copy data from the use provided buffer to the pagecache pages and that can
cause a page fault on the user provided buffer which will try to grab
task->mm->mmap_lock.
This lock inversion is the main reason why inode lock cannot be used
anywhere in the page fault path.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-30 7:42 ` Jan Kara
@ 2023-05-30 9:39 ` Baokun Li
2023-05-30 10:28 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Baokun Li @ 2023-05-30 9:39 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable, Baokun Li
On 2023/5/30 15:42, Jan Kara wrote:
> On Tue 30-05-23 10:00:44, Baokun Li wrote:
>> On 2023/5/29 22:44, Jan Kara wrote:
>>> On Mon 29-05-23 16:01:48, Baokun Li wrote:
>>>> Syzbot reported a BUG_ON:
>>>> ==================================================================
>>>> EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
>>>> EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
>>>> bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
>>>> ------------[ cut here ]------------
>>>> kernel BUG at fs/ext4/ext4_jbd2.c:53!
>>>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>>>> CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
>>>> RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
>>>> [...]
>>>> Call Trace:
>>>> ext4_write_inline_data_end+0xa39/0xdf0
>>>> ext4_da_write_end+0x1e2/0x950
>>>> generic_perform_write+0x401/0x5f0
>>>> ext4_buffered_write_iter+0x35f/0x640
>>>> ext4_file_write_iter+0x198/0x1cd0
>>>> vfs_write+0x8b5/0xef0
>>>> [...]
>>>> ==================================================================
>>>>
>>>> The above BUG_ON is triggered by the following race:
>>>>
>>>> cpu1 cpu2
>>>> ________________________|________________________
>>>> ksys_write
>>>> vfs_write
>>>> new_sync_write
>>>> ext4_file_write_iter
>>>> ext4_buffered_write_iter
>>>> generic_perform_write
>>>> ext4_da_write_begin
>>>> do_fault
>>>> do_page_mkwrite
>>>> ext4_page_mkwrite
>>>> ext4_convert_inline_data
>>>> ext4_convert_inline_data_nolock
>>>> ext4_destroy_inline_data_nolock
>>>> //clear EXT4_STATE_MAY_INLINE_DATA
>>>> ext4_map_blocks --> return error
>>>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>>>> ext4_block_write_begin
>>>> ext4_restore_inline_data
>>>> // set EXT4_STATE_MAY_INLINE_DATA
>>>> ext4_da_write_end
>>>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>>>> ext4_write_inline_data_end
>>>> handle=NULL
>>>> ext4_journal_stop(handle)
>>>> __ext4_journal_stop
>>>> ext4_put_nojournal(handle)
>>>> ref_cnt = (unsigned long)handle
>>>> BUG_ON(ref_cnt == 0) ---> BUG_ON
>>>>
>>>> The root cause of this problem is that the ext4_convert_inline_data() in
>>>> ext4_page_mkwrite() does not grab i_rwsem, so it may race with
>>>> ext4_buffered_write_iter() and cause the write_begin() and write_end()
>>>> functions to be inconsistent and trigger BUG_ON.
>>>>
>>>> To solve the above issue, we cannot add inode_lock directly to
>>>> ext4_page_mkwrite(), because this function is a hot path and frequent calls
>>>> to inode_lock will cause performance degradation for multi-threaded reads
>>>> and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
>>>> and only when inline_data is enabled and mmap a file in shared write mode,
>>>> we hold the lock to convert, which can reduce the impact on performance.
>>>>
>>>> Reported-by: Jun Nie <jun.nie@linaro.org>
>>>> Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
>>>> Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
>>>> Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
>>>> CC: stable@vger.kernel.org # 4.12+
>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>> Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
>>> not so much about performance as about lock ordering. In
>>> ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
>>> acquire i_rwsem because it ranks about it.
>> Thank you for your review!
>>
>> I'm sorry I didn't make myself clear here.
>>
>> Yes, we can't get i_rwsem after holding mmap_sem at any time, otherwise
>> ABBA deadlock may occur. The "add inode_lock directly" in my patch
>> description actually looks like this:
>> ```
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index b98d2d58b900..c9318dc2a613 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -6025,12 +6025,14 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>> sb_start_pagefault(inode->i_sb);
>> file_update_time(vma->vm_file);
>>
>> - filemap_invalidate_lock_shared(mapping);
>> -
>> + inode_lock(inode);
>> err = ext4_convert_inline_data(inode);
>> + inode_unlock(inode);
>> if (err)
>> goto out_ret;
>>
>> + filemap_invalidate_lock_shared(mapping);
>> +
>> /*
>> * On data journalling we skip straight to the transaction handle:
>> * there's no delalloc; page truncated will be checked later; the
>> ```
> Yes, but even this could deadlock. The ABBA deadlock I'm speaking about
> would not be created with mapping->invalidate_lock but rather with
> task->mm->mmap_lock which is acquired at the beginning of page fault in the
> arch code.
Thanks for the explanation!
I thought the mmap_sem said was "&EXT4_I(inode)->i_mmap_sem".
But here are some more questions:
1) In the arch code page fault is handled by mmap_read_lock(mm) to get the
shared lock, why would this lead to ABBA deadlock?
2) Why would page fault be triggered in the write process?
Could you explain it in more detail?
> And when you do write(2), you hold inode_lock() and then you
> copy data from the use provided buffer to the pagecache pages and that can
> cause a page fault on the user provided buffer which will try to grab
> task->mm->mmap_lock.
>
> This lock inversion is the main reason why inode lock cannot be used
> anywhere in the page fault path.
>
> Honza
>
Looking forward to hearing from you! 🤔
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-30 9:39 ` Baokun Li
@ 2023-05-30 10:28 ` Jan Kara
2023-05-30 13:19 ` Baokun Li
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2023-05-30 10:28 UTC (permalink / raw)
To: Baokun Li
Cc: Jan Kara, linux-ext4, tytso, adilger.kernel, ritesh.list,
linux-kernel, jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable
On Tue 30-05-23 17:39:16, Baokun Li wrote:
> On 2023/5/30 15:42, Jan Kara wrote:
> > On Tue 30-05-23 10:00:44, Baokun Li wrote:
> > > On 2023/5/29 22:44, Jan Kara wrote:
> > > > On Mon 29-05-23 16:01:48, Baokun Li wrote:
> > > > > Syzbot reported a BUG_ON:
> > > > > ==================================================================
> > > > > EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
> > > > > EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
> > > > > bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
> > > > > ------------[ cut here ]------------
> > > > > kernel BUG at fs/ext4/ext4_jbd2.c:53!
> > > > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > > > CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
> > > > > RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
> > > > > [...]
> > > > > Call Trace:
> > > > > ext4_write_inline_data_end+0xa39/0xdf0
> > > > > ext4_da_write_end+0x1e2/0x950
> > > > > generic_perform_write+0x401/0x5f0
> > > > > ext4_buffered_write_iter+0x35f/0x640
> > > > > ext4_file_write_iter+0x198/0x1cd0
> > > > > vfs_write+0x8b5/0xef0
> > > > > [...]
> > > > > ==================================================================
> > > > >
> > > > > The above BUG_ON is triggered by the following race:
> > > > >
> > > > > cpu1 cpu2
> > > > > ________________________|________________________
> > > > > ksys_write
> > > > > vfs_write
> > > > > new_sync_write
> > > > > ext4_file_write_iter
> > > > > ext4_buffered_write_iter
> > > > > generic_perform_write
> > > > > ext4_da_write_begin
> > > > > do_fault
> > > > > do_page_mkwrite
> > > > > ext4_page_mkwrite
> > > > > ext4_convert_inline_data
> > > > > ext4_convert_inline_data_nolock
> > > > > ext4_destroy_inline_data_nolock
> > > > > //clear EXT4_STATE_MAY_INLINE_DATA
> > > > > ext4_map_blocks --> return error
> > > > > ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> > > > > ext4_block_write_begin
> > > > > ext4_restore_inline_data
> > > > > // set EXT4_STATE_MAY_INLINE_DATA
> > > > > ext4_da_write_end
> > > > > ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
> > > > > ext4_write_inline_data_end
> > > > > handle=NULL
> > > > > ext4_journal_stop(handle)
> > > > > __ext4_journal_stop
> > > > > ext4_put_nojournal(handle)
> > > > > ref_cnt = (unsigned long)handle
> > > > > BUG_ON(ref_cnt == 0) ---> BUG_ON
> > > > >
> > > > > The root cause of this problem is that the ext4_convert_inline_data() in
> > > > > ext4_page_mkwrite() does not grab i_rwsem, so it may race with
> > > > > ext4_buffered_write_iter() and cause the write_begin() and write_end()
> > > > > functions to be inconsistent and trigger BUG_ON.
> > > > >
> > > > > To solve the above issue, we cannot add inode_lock directly to
> > > > > ext4_page_mkwrite(), because this function is a hot path and frequent calls
> > > > > to inode_lock will cause performance degradation for multi-threaded reads
> > > > > and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
> > > > > and only when inline_data is enabled and mmap a file in shared write mode,
> > > > > we hold the lock to convert, which can reduce the impact on performance.
> > > > >
> > > > > Reported-by: Jun Nie <jun.nie@linaro.org>
> > > > > Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
> > > > > Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
> > > > > Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
> > > > > CC: stable@vger.kernel.org # 4.12+
> > > > > Signed-off-by: Baokun Li <libaokun1@huawei.com>
> > > > Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
> > > > not so much about performance as about lock ordering. In
> > > > ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
> > > > acquire i_rwsem because it ranks about it.
> > > Thank you for your review!
> > >
> > > I'm sorry I didn't make myself clear here.
> > >
> > > Yes, we can't get i_rwsem after holding mmap_sem at any time, otherwise
> > > ABBA deadlock may occur. The "add inode_lock directly" in my patch
> > > description actually looks like this:
> > > ```
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > index b98d2d58b900..c9318dc2a613 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -6025,12 +6025,14 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
> > > sb_start_pagefault(inode->i_sb);
> > > file_update_time(vma->vm_file);
> > >
> > > - filemap_invalidate_lock_shared(mapping);
> > > -
> > > + inode_lock(inode);
> > > err = ext4_convert_inline_data(inode);
> > > + inode_unlock(inode);
> > > if (err)
> > > goto out_ret;
> > >
> > > + filemap_invalidate_lock_shared(mapping);
> > > +
> > > /*
> > > * On data journalling we skip straight to the transaction handle:
> > > * there's no delalloc; page truncated will be checked later; the
> > > ```
> > Yes, but even this could deadlock. The ABBA deadlock I'm speaking about
> > would not be created with mapping->invalidate_lock but rather with
> > task->mm->mmap_lock which is acquired at the beginning of page fault in the
> > arch code.
>
> Thanks for the explanation!
>
> I thought the mmap_sem said was "&EXT4_I(inode)->i_mmap_sem".
We don't have that one anymore :). But yes, there are too many mmap locks so
I should have been more precise from the start. I'm sorry for the
confusion.
> But here are some more questions:
>
> 1) In the arch code page fault is handled by mmap_read_lock(mm) to get the
> shared lock, why would this lead to ABBA deadlock?
>
> 2) Why would page fault be triggered in the write process?
>
> Could you explain it in more detail?
OK, let me draw here the exact deadlock graph:
CPU0 CPU1 CPU2
write(2) page fault mmap(2)
do_user_addr_fault()
mmap_read_lock() ...
... vm_mmap_pgoff()
mmap_write_lock_killable(mm)
- blocks on CPU1
holding the lock
ext4_buffered_write_iter()
inode_lock(inode);
...
generic_perform_write(iocb, from);
fault_in_iov_iter_readable(i, bytes)
- hits pagefault
do_user_addr_fault()
mmap_read_lock()
- blocks on CPU2 as writer queued first
ext4_page_mkwrite()
- would deadlock here if we
try to take inode lock
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ext4: fix race condition between buffer write and page_mkwrite
2023-05-30 10:28 ` Jan Kara
@ 2023-05-30 13:19 ` Baokun Li
0 siblings, 0 replies; 7+ messages in thread
From: Baokun Li @ 2023-05-30 13:19 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
jun.nie, ebiggers, yi.zhang, yangerkun, yukuai3,
syzbot+a158d886ca08a3fecca4, stable, Baokun Li
On 2023/5/30 18:28, Jan Kara wrote:
> On Tue 30-05-23 17:39:16, Baokun Li wrote:
>> On 2023/5/30 15:42, Jan Kara wrote:
>>> On Tue 30-05-23 10:00:44, Baokun Li wrote:
>>>> On 2023/5/29 22:44, Jan Kara wrote:
>>>>> On Mon 29-05-23 16:01:48, Baokun Li wrote:
>>>>>> Syzbot reported a BUG_ON:
>>>>>> ==================================================================
>>>>>> EXT4-fs (loop0): mounted filesystem without journal. Quota mode: none.
>>>>>> EXT4-fs error (device loop0): ext4_mb_generate_buddy:1098: group 0, block
>>>>>> bitmap and bg descriptor inconsistent: 25 vs 150994969 free clusters
>>>>>> ------------[ cut here ]------------
>>>>>> kernel BUG at fs/ext4/ext4_jbd2.c:53!
>>>>>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>>>>>> CPU: 1 PID: 494 Comm: syz-executor.0 6.1.0-rc7-syzkaller-ga4412fdd49dc #0
>>>>>> RIP: 0010:__ext4_journal_stop+0x1b3/0x1c0
>>>>>> [...]
>>>>>> Call Trace:
>>>>>> ext4_write_inline_data_end+0xa39/0xdf0
>>>>>> ext4_da_write_end+0x1e2/0x950
>>>>>> generic_perform_write+0x401/0x5f0
>>>>>> ext4_buffered_write_iter+0x35f/0x640
>>>>>> ext4_file_write_iter+0x198/0x1cd0
>>>>>> vfs_write+0x8b5/0xef0
>>>>>> [...]
>>>>>> ==================================================================
>>>>>>
>>>>>> The above BUG_ON is triggered by the following race:
>>>>>>
>>>>>> cpu1 cpu2
>>>>>> ________________________|________________________
>>>>>> ksys_write
>>>>>> vfs_write
>>>>>> new_sync_write
>>>>>> ext4_file_write_iter
>>>>>> ext4_buffered_write_iter
>>>>>> generic_perform_write
>>>>>> ext4_da_write_begin
>>>>>> do_fault
>>>>>> do_page_mkwrite
>>>>>> ext4_page_mkwrite
>>>>>> ext4_convert_inline_data
>>>>>> ext4_convert_inline_data_nolock
>>>>>> ext4_destroy_inline_data_nolock
>>>>>> //clear EXT4_STATE_MAY_INLINE_DATA
>>>>>> ext4_map_blocks --> return error
>>>>>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>>>>>> ext4_block_write_begin
>>>>>> ext4_restore_inline_data
>>>>>> // set EXT4_STATE_MAY_INLINE_DATA
>>>>>> ext4_da_write_end
>>>>>> ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)
>>>>>> ext4_write_inline_data_end
>>>>>> handle=NULL
>>>>>> ext4_journal_stop(handle)
>>>>>> __ext4_journal_stop
>>>>>> ext4_put_nojournal(handle)
>>>>>> ref_cnt = (unsigned long)handle
>>>>>> BUG_ON(ref_cnt == 0) ---> BUG_ON
>>>>>>
>>>>>> The root cause of this problem is that the ext4_convert_inline_data() in
>>>>>> ext4_page_mkwrite() does not grab i_rwsem, so it may race with
>>>>>> ext4_buffered_write_iter() and cause the write_begin() and write_end()
>>>>>> functions to be inconsistent and trigger BUG_ON.
>>>>>>
>>>>>> To solve the above issue, we cannot add inode_lock directly to
>>>>>> ext4_page_mkwrite(), because this function is a hot path and frequent calls
>>>>>> to inode_lock will cause performance degradation for multi-threaded reads
>>>>>> and writes. Hence, we move ext4_convert_inline_data() to ext4_file_mmap(),
>>>>>> and only when inline_data is enabled and mmap a file in shared write mode,
>>>>>> we hold the lock to convert, which can reduce the impact on performance.
>>>>>>
>>>>>> Reported-by: Jun Nie <jun.nie@linaro.org>
>>>>>> Closes: https://lore.kernel.org/lkml/63903521.5040307@huawei.com/t/
>>>>>> Reported-by: syzbot+a158d886ca08a3fecca4@syzkaller.appspotmail.com
>>>>>> Closes: https://syzkaller.appspot.com/bug?id=899b37f20ce4072bcdfecfe1647b39602e956e36
>>>>>> Fixes: 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map")
>>>>>> CC: stable@vger.kernel.org # 4.12+
>>>>>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>>>>> Thanks for the patch! The problem with i_rwsem in ext4_page_mkwrite() is
>>>>> not so much about performance as about lock ordering. In
>>>>> ext4_page_mkwrite() we are called with mmap_sem held and so we cannot
>>>>> acquire i_rwsem because it ranks about it.
>>>> Thank you for your review!
>>>>
>>>> I'm sorry I didn't make myself clear here.
>>>>
>>>> Yes, we can't get i_rwsem after holding mmap_sem at any time, otherwise
>>>> ABBA deadlock may occur. The "add inode_lock directly" in my patch
>>>> description actually looks like this:
>>>> ```
>>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>>> index b98d2d58b900..c9318dc2a613 100644
>>>> --- a/fs/ext4/inode.c
>>>> +++ b/fs/ext4/inode.c
>>>> @@ -6025,12 +6025,14 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>>>> sb_start_pagefault(inode->i_sb);
>>>> file_update_time(vma->vm_file);
>>>>
>>>> - filemap_invalidate_lock_shared(mapping);
>>>> -
>>>> + inode_lock(inode);
>>>> err = ext4_convert_inline_data(inode);
>>>> + inode_unlock(inode);
>>>> if (err)
>>>> goto out_ret;
>>>>
>>>> + filemap_invalidate_lock_shared(mapping);
>>>> +
>>>> /*
>>>> * On data journalling we skip straight to the transaction handle:
>>>> * there's no delalloc; page truncated will be checked later; the
>>>> ```
>>> Yes, but even this could deadlock. The ABBA deadlock I'm speaking about
>>> would not be created with mapping->invalidate_lock but rather with
>>> task->mm->mmap_lock which is acquired at the beginning of page fault in the
>>> arch code.
>> Thanks for the explanation!
>>
>> I thought the mmap_sem said was "&EXT4_I(inode)->i_mmap_sem".
> We don't have that one anymore :). But yes, there are too many mmap locks so
> I should have been more precise from the start. I'm sorry for the
> confusion.
>
>> But here are some more questions:
>>
>> 1) In the arch code page fault is handled by mmap_read_lock(mm) to get the
>> shared lock, why would this lead to ABBA deadlock?
>>
>> 2) Why would page fault be triggered in the write process?
>>
>> Could you explain it in more detail?
> OK, let me draw here the exact deadlock graph:
>
> CPU0 CPU1 CPU2
> write(2) page fault mmap(2)
> do_user_addr_fault()
> mmap_read_lock() ...
> ... vm_mmap_pgoff()
> mmap_write_lock_killable(mm)
> - blocks on CPU1
> holding the lock
> ext4_buffered_write_iter()
> inode_lock(inode);
> ...
> generic_perform_write(iocb, from);
> fault_in_iov_iter_readable(i, bytes)
> - hits pagefault
> do_user_addr_fault()
> mmap_read_lock()
> - blocks on CPU2 as writer queued first
> ext4_page_mkwrite()
> - would deadlock here if we
> try to take inode lock
>
> Honza
Got it! This graph is very clear! 👍
Thank you very much for your patient and detailed explanation!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-30 13:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-29 8:01 [PATCH] ext4: fix race condition between buffer write and page_mkwrite Baokun Li
2023-05-29 14:44 ` Jan Kara
2023-05-30 2:00 ` Baokun Li
2023-05-30 7:42 ` Jan Kara
2023-05-30 9:39 ` Baokun Li
2023-05-30 10:28 ` Jan Kara
2023-05-30 13:19 ` Baokun Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.