* [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer
@ 2018-05-05 10:02 Chao Yu
2018-05-05 14:55 ` Chao Yu
0 siblings, 1 reply; 3+ messages in thread
From: Chao Yu @ 2018-05-05 10:02 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
Thread A Thread B Thread C
- f2fs_remount
- stop_gc_thread
- f2fs_sbi_store
- issue_discard_thread
sbi->gc_thread = NULL;
sbi->gc_thread->gc_wake = 1
access sbi->gc_thread->gc_urgent
Previously, we allocate memory for sbi->gc_thread based on background
gc thread mount option, the memory can be released if we turn off
that mount option, but still there are several places access gc_thread
pointer without considering race condition, result in NULL point
dereference.
In order to fix this issue, use sb->s_umount to exclude those operations
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v3:
- use sb->s_umount to make all race cases exclusive.
fs/f2fs/gc.c | 4 ++++
fs/f2fs/segment.c | 11 ++++++++++-
fs/f2fs/sysfs.c | 14 +++++++++++---
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 9bb2ddbbed1e..d7d469f9be0a 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -187,17 +187,21 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
p->max_search = dirty_i->nr_dirty[type];
p->ofs_unit = 1;
} else {
+ down_read(&sbi->sb->s_umount);
p->gc_mode = select_gc_type(sbi->gc_thread, gc_type);
+ up_read(&sbi->sb->s_umount);
p->dirty_segmap = dirty_i->dirty_segmap[DIRTY];
p->max_search = dirty_i->nr_dirty[DIRTY];
p->ofs_unit = sbi->segs_per_sec;
}
/* we need to check every dirty segments in the FG_GC case */
+ down_read(&sbi->sb->s_umount);
if (gc_type != FG_GC &&
(sbi->gc_thread && !sbi->gc_thread->gc_urgent) &&
p->max_search > sbi->max_victim_search)
p->max_search = sbi->max_victim_search;
+ up_read(&sbi->sb->s_umount);
/* let's select beginning hot/small space first in no_heap mode*/
if (test_opt(sbi, NOHEAP) &&
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 320cc1c57246..74e184ab0544 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -174,11 +174,18 @@ bool need_SSR(struct f2fs_sb_info *sbi)
int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES);
int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS);
int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA);
+ bool gc_urgent = false;
if (test_opt(sbi, LFS))
return false;
+
+ down_read(&sbi->sb->s_umount);
if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
- return true;
+ gc_urgent = true;
+ up_read(&sbi->sb->s_umount);
+
+ if (gc_urgent)
+ return false;
return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs +
SM_I(sbi)->min_ssr_sections + reserved_sections(sbi));
@@ -1421,8 +1428,10 @@ static int issue_discard_thread(void *data)
if (dcc->discard_wake)
dcc->discard_wake = 0;
+ down_read(&sbi->sb->s_umount);
if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
init_discard_policy(&dpolicy, DPOLICY_FORCE, 1);
+ up_read(&sbi->sb->s_umount);
sb_start_intwrite(sbi->sb);
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 6d8d8f41e517..1cba68812b32 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -173,6 +173,7 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
unsigned long t;
unsigned int *ui;
ssize_t ret;
+ bool gc_entry = (a->struct_type == GC_THREAD);
ptr = __struct_ptr(sbi, a->struct_type);
if (!ptr)
@@ -248,16 +249,23 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
if (!strcmp(a->attr.name, "trim_sections"))
return -EINVAL;
- *ui = t;
-
- if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0)
+ if (!strcmp(a->attr.name, "iostat_enable") && t == 0)
f2fs_reset_iostat(sbi);
+
+ if (gc_entry)
+ down_read(&sbi->sb->s_umount);
+
if (!strcmp(a->attr.name, "gc_urgent") && t == 1 && sbi->gc_thread) {
sbi->gc_thread->gc_wake = 1;
wake_up_interruptible_all(&sbi->gc_thread->gc_wait_queue_head);
wake_up_discard_thread(sbi, true);
}
+ *ui = t;
+
+ if (gc_entry)
+ up_read(&sbi->sb->s_umount);
+
return count;
}
--
2.17.0.391.g1f1cddd558b5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer
2018-05-05 10:02 [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer Chao Yu
@ 2018-05-05 14:55 ` Chao Yu
0 siblings, 0 replies; 3+ messages in thread
From: Chao Yu @ 2018-05-05 14:55 UTC (permalink / raw)
To: Chao Yu, jaegeuk; +Cc: linux-f2fs-devel, linux-kernel
On 2018/5/5 18:02, Chao Yu wrote:
> Thread A Thread B Thread C
> - f2fs_remount
> - stop_gc_thread
> - f2fs_sbi_store
> - issue_discard_thread
> sbi->gc_thread = NULL;
> sbi->gc_thread->gc_wake = 1
> access sbi->gc_thread->gc_urgent
>
> Previously, we allocate memory for sbi->gc_thread based on background
> gc thread mount option, the memory can be released if we turn off
> that mount option, but still there are several places access gc_thread
> pointer without considering race condition, result in NULL point
> dereference.
>
> In order to fix this issue, use sb->s_umount to exclude those operations
I encounter deadlock with this patch
dump_stack+0x5f/0x86
__lock_acquire+0xff7/0x10e0
lock_acquire+0xae/0x220
down_read+0x38/0x60 try lock s_umount again
need_SSR+0x5d/0x160 [f2fs]
allocate_segment_by_default+0xb7/0x1c0 [f2fs]
allocate_data_block+0x183/0x4c0 [f2fs]
do_write_page+0x52/0x80 [f2fs]
write_data_page+0x4a/0xd0 [f2fs]
do_write_data_page+0x327/0x630 [f2fs]
__write_data_page+0x34b/0x800 [f2fs]
__f2fs_write_data_pages+0x3f1/0x8e0 [f2fs]
f2fs_write_data_pages+0x27/0x30 [f2fs]
do_writepages+0x1a/0x70
__writeback_single_inode+0x55/0x7e0
writeback_sb_inodes+0x21b/0x490
__writeback_inodes_wb+0x7c/0xb0 trylock_super has alread hold s_umount
wb_writeback+0x3e2/0x580
wb_workfn+0x251/0x6b0
process_one_work+0x196/0x550
worker_thread+0x31/0x360
kthread+0xe3/0x110
ret_from_fork+0x2e/0x38
So, is it better to introduce private lock to avoid the race condition?
Thanks,
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v3:
> - use sb->s_umount to make all race cases exclusive.
> fs/f2fs/gc.c | 4 ++++
> fs/f2fs/segment.c | 11 ++++++++++-
> fs/f2fs/sysfs.c | 14 +++++++++++---
> 3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 9bb2ddbbed1e..d7d469f9be0a 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -187,17 +187,21 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
> p->max_search = dirty_i->nr_dirty[type];
> p->ofs_unit = 1;
> } else {
> + down_read(&sbi->sb->s_umount);
> p->gc_mode = select_gc_type(sbi->gc_thread, gc_type);
> + up_read(&sbi->sb->s_umount);
> p->dirty_segmap = dirty_i->dirty_segmap[DIRTY];
> p->max_search = dirty_i->nr_dirty[DIRTY];
> p->ofs_unit = sbi->segs_per_sec;
> }
>
> /* we need to check every dirty segments in the FG_GC case */
> + down_read(&sbi->sb->s_umount);
> if (gc_type != FG_GC &&
> (sbi->gc_thread && !sbi->gc_thread->gc_urgent) &&
> p->max_search > sbi->max_victim_search)
> p->max_search = sbi->max_victim_search;
> + up_read(&sbi->sb->s_umount);
>
> /* let's select beginning hot/small space first in no_heap mode*/
> if (test_opt(sbi, NOHEAP) &&
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 320cc1c57246..74e184ab0544 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -174,11 +174,18 @@ bool need_SSR(struct f2fs_sb_info *sbi)
> int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES);
> int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS);
> int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA);
> + bool gc_urgent = false;
>
> if (test_opt(sbi, LFS))
> return false;
> +
> + down_read(&sbi->sb->s_umount);
> if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
> - return true;
> + gc_urgent = true;
> + up_read(&sbi->sb->s_umount);
> +
> + if (gc_urgent)
> + return false;
>
> return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs +
> SM_I(sbi)->min_ssr_sections + reserved_sections(sbi));
> @@ -1421,8 +1428,10 @@ static int issue_discard_thread(void *data)
> if (dcc->discard_wake)
> dcc->discard_wake = 0;
>
> + down_read(&sbi->sb->s_umount);
> if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
> init_discard_policy(&dpolicy, DPOLICY_FORCE, 1);
> + up_read(&sbi->sb->s_umount);
>
> sb_start_intwrite(sbi->sb);
>
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 6d8d8f41e517..1cba68812b32 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -173,6 +173,7 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
> unsigned long t;
> unsigned int *ui;
> ssize_t ret;
> + bool gc_entry = (a->struct_type == GC_THREAD);
>
> ptr = __struct_ptr(sbi, a->struct_type);
> if (!ptr)
> @@ -248,16 +249,23 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
> if (!strcmp(a->attr.name, "trim_sections"))
> return -EINVAL;
>
> - *ui = t;
> -
> - if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0)
> + if (!strcmp(a->attr.name, "iostat_enable") && t == 0)
> f2fs_reset_iostat(sbi);
> +
> + if (gc_entry)
> + down_read(&sbi->sb->s_umount);
> +
> if (!strcmp(a->attr.name, "gc_urgent") && t == 1 && sbi->gc_thread) {
> sbi->gc_thread->gc_wake = 1;
> wake_up_interruptible_all(&sbi->gc_thread->gc_wait_queue_head);
> wake_up_discard_thread(sbi, true);
> }
>
> + *ui = t;
> +
> + if (gc_entry)
> + up_read(&sbi->sb->s_umount);
> +
> return count;
> }
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer
@ 2018-05-28 8:57 Chao Yu
0 siblings, 0 replies; 3+ messages in thread
From: Chao Yu @ 2018-05-28 8:57 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
Thread A Thread B
- f2fs_remount
- stop_gc_thread
- f2fs_sbi_store
sbi->gc_thread = NULL;
access sbi->gc_thread->gc_*
Previously, we allocate memory for sbi->gc_thread based on background
gc thread mount option, the memory can be released if we turn off
that mount option, but still there are several places access gc_thread
pointer without considering race condition, result in NULL point
dereference.
In order to fix this issue, use sb->s_umount to exclude those operations.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v3:
- fix missing 'gc_urgent' case
fs/f2fs/sysfs.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index dd940d156af6..ac3ea6044936 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -165,7 +165,7 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
return snprintf(buf, PAGE_SIZE, "%u\n", *ui);
}
-static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
+static ssize_t __f2fs_sbi_store(struct f2fs_attr *a,
struct f2fs_sb_info *sbi,
const char *buf, size_t count)
{
@@ -278,6 +278,23 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
return count;
}
+static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
+ struct f2fs_sb_info *sbi,
+ const char *buf, size_t count)
+{
+ ssize_t ret;
+ bool gc_entry = (!strcmp(a->attr.name, "gc_urgent") ||
+ a->struct_type == GC_THREAD);
+
+ if (gc_entry)
+ down_read(&sbi->sb->s_umount);
+ ret = __f2fs_sbi_store(a, sbi, buf, count);
+ if (gc_entry)
+ up_read(&sbi->sb->s_umount);
+
+ return ret;
+}
+
static ssize_t f2fs_attr_show(struct kobject *kobj,
struct attribute *attr, char *buf)
{
--
2.17.0.391.g1f1cddd558b5
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-28 8:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-05 10:02 [PATCH v3] f2fs: fix to avoid race during access gc_thread pointer Chao Yu
2018-05-05 14:55 ` Chao Yu
-- strict thread matches above, loose matches on Subject: below --
2018-05-28 8:57 Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).