| 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
| ||
|
Date: Tue, 8 May 2018 11:21:40 +0800
From: Chao Yu <yuchao0@...wei.com>
To: Jaegeuk Kim <jaegeuk@...nel.org>
CC: <linux-f2fs-devel@...ts.sourceforge.net>,
<linux-kernel@...r.kernel.org>, <chao@...nel.org>
Subject: Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread
pointer
On 2018/5/8 11:17, Jaegeuk Kim wrote:
> On 05/08, Chao Yu wrote:
>> On 2018/5/8 5:36, Jaegeuk Kim wrote:
>>> On 05/07, 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, introduce gc_rwsem to exclude those operations.
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@...wei.com>
>>>> ---
>>>> v4:
>>>> - use introduced sbi.gc_rwsem lock instead of sb.s_umount.
>>>
>>> We can use this first.
>>>
>>> >From e62e8d3ece6ee8a4aeac8ffd6161d25851f8b3f0 Mon Sep 17 00:00:00 2001
>>> From: Jaegeuk Kim <jaegeuk@...nel.org>
>>> Date: Mon, 7 May 2018 14:22:40 -0700
>>> Subject: [PATCH] f2fs: introduce sbi->gc_mode to determine the policy
>>>
>>> This is to avoid sbi->gc_thread pointer access.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.org>
>>> ---
>>> fs/f2fs/f2fs.h | 8 ++++++++
>>> fs/f2fs/gc.c | 28 ++++++++++++----------------
>>> fs/f2fs/gc.h | 2 --
>>> fs/f2fs/segment.c | 4 ++--
>>> fs/f2fs/sysfs.c | 33 +++++++++++++++++++++++++--------
>>> 5 files changed, 47 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 80490a7991a7..779d8b26878c 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -1065,6 +1065,13 @@ enum {
>>> MAX_TIME,
>>> };
>>>
>>> +enum {
>>> + GC_NORMAL,
>>> + GC_IDLE_CB,
>>> + GC_IDLE_GREEDY,
>>> + GC_URGENT,
>>> +};
>>> +
>>> enum {
>>> WHINT_MODE_OFF, /* not pass down write hints */
>>> WHINT_MODE_USER, /* try to pass down hints given by users */
>>> @@ -1193,6 +1200,7 @@ struct f2fs_sb_info {
>>> struct mutex gc_mutex; /* mutex for GC */
>>> struct f2fs_gc_kthread *gc_thread; /* GC thread */
>>> unsigned int cur_victim_sec; /* current victim section num */
>>> + unsigned int gc_mode; /* current GC state */
>>>
>>> /* threshold for gc trials on pinned files */
>>> u64 gc_pin_file_threshold;
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 9bb2ddbbed1e..7ec8ea75dfde 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -76,7 +76,7 @@ static int gc_thread_func(void *data)
>>> * invalidated soon after by user update or deletion.
>>> * So, I'd like to wait some time to collect dirty segments.
>>> */
>>> - if (gc_th->gc_urgent) {
>>> + if (sbi->gc_mode == GC_URGENT) {
>>> wait_ms = gc_th->urgent_sleep_time;
>>> mutex_lock(&sbi->gc_mutex);
>>> goto do_gc;
>>> @@ -131,8 +131,6 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
>>> gc_th->max_sleep_time = DEF_GC_THREAD_MAX_SLEEP_TIME;
>>> gc_th->no_gc_sleep_time = DEF_GC_THREAD_NOGC_SLEEP_TIME;
>>>
>>> - gc_th->gc_idle = 0;
>>> - gc_th->gc_urgent = 0;
>>> gc_th->gc_wake= 0;
>>>
>>> sbi->gc_thread = gc_th;
>>> @@ -158,21 +156,19 @@ void stop_gc_thread(struct f2fs_sb_info *sbi)
>>> sbi->gc_thread = NULL;
>>> }
>>>
>>> -static int select_gc_type(struct f2fs_gc_kthread *gc_th, int gc_type)
>>> +static int select_gc_type(struct f2fs_sb_info *sbi, int gc_type)
>>> {
>>> int gc_mode = (gc_type == BG_GC) ? GC_CB : GC_GREEDY;
>>>
>>> - if (!gc_th)
>>> - return gc_mode;
>>> -
>>> - if (gc_th->gc_idle) {
>>> - if (gc_th->gc_idle == 1)
>>> - gc_mode = GC_CB;
>>> - else if (gc_th->gc_idle == 2)
>>> - gc_mode = GC_GREEDY;
>>> - }
>>> - if (gc_th->gc_urgent)
>>> + switch (sbi->gc_mode) {
>>> + case GC_IDLE_CB:
>>> + gc_mode = GC_CB;
>>> + break;
>>> + case GC_IDLE_GREEDY:
>>> + case GC_URGENT:
>>> gc_mode = GC_GREEDY;
>>> + break;
>>> + }
>>> return gc_mode;
>>> }
>>>
>>> @@ -187,7 +183,7 @@ 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 {
>>> - p->gc_mode = select_gc_type(sbi->gc_thread, gc_type);
>>> + p->gc_mode = select_gc_type(sbi, gc_type);
>>> p->dirty_segmap = dirty_i->dirty_segmap[DIRTY];
>>> p->max_search = dirty_i->nr_dirty[DIRTY];
>>> p->ofs_unit = sbi->segs_per_sec;
>>> @@ -195,7 +191,7 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
>>>
>>> /* we need to check every dirty segments in the FG_GC case */
>>> if (gc_type != FG_GC &&
>>> - (sbi->gc_thread && !sbi->gc_thread->gc_urgent) &&
>>> + (sbi->gc_mode != GC_URGENT) &&
>>> p->max_search > sbi->max_victim_search)
>>> p->max_search = sbi->max_victim_search;
>>>
>>> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
>>> index b0045d4c8d1e..c8619e408009 100644
>>> --- a/fs/f2fs/gc.h
>>> +++ b/fs/f2fs/gc.h
>>> @@ -36,8 +36,6 @@ struct f2fs_gc_kthread {
>>> unsigned int no_gc_sleep_time;
>>>
>>> /* for changing gc mode */
>>> - unsigned int gc_idle;
>>> - unsigned int gc_urgent;
>>> unsigned int gc_wake;
>>> };
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 503a98abdb2f..5a7ed06291e0 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -177,7 +177,7 @@ bool need_SSR(struct f2fs_sb_info *sbi)
>>>
>>> if (test_opt(sbi, LFS))
>>> return false;
>>> - if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
>>> + if (sbi->gc_mode == GC_URGENT)
>>> return true;
>>>
>>> return free_sections(sbi) <= (node_secs + 2 * dent_secs + imeta_secs +
>>> @@ -1418,7 +1418,7 @@ static int issue_discard_thread(void *data)
>>> if (dcc->discard_wake)
>>> dcc->discard_wake = 0;
>>>
>>> - if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
>>> + if (sbi->gc_mode == GC_URGENT)
>>> init_discard_policy(&dpolicy, DPOLICY_FORCE, 1);
>>>
>>> sb_start_intwrite(sbi->sb);
>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>> index 6d8d8f41e517..dd940d156af6 100644
>>> --- a/fs/f2fs/sysfs.c
>>> +++ b/fs/f2fs/sysfs.c
>>> @@ -248,16 +248,33 @@ static ssize_t f2fs_sbi_store(struct f2fs_attr *a,
>>> if (!strcmp(a->attr.name, "trim_sections"))
>>> return -EINVAL;
>>>
>>> + if (!strcmp(a->attr.name, "gc_urgent")) {
>>> + if (t >= 1) {
>>> + sbi->gc_mode = GC_URGENT;
>>> + if (sbi->gc_thread) {
>>> + wake_up_interruptible_all(
>>> + &sbi->gc_thread->gc_wait_queue_head);
>>> + wake_up_discard_thread(sbi, true);
>>> + }
>>> + } else {
>>> + sbi->gc_mode = GC_NORMAL;
>>> + }
>>> + return count;
>>> + }
>>> + if (!strcmp(a->attr.name, "gc_idle")) {
>>> + if (t == GC_IDLE_CB)
>>> + sbi->gc_mode = GC_IDLE_CB;
>>> + else if (t == GC_IDLE_GREEDY)
>>> + sbi->gc_mode = GC_IDLE_GREEDY;
>>> + else
>>> + sbi->gc_mode = GC_NORMAL;
>>> + return count;
>>> + }
>>> +
>>> *ui = t;
>>>
>>> if (!strcmp(a->attr.name, "iostat_enable") && *ui == 0)
>>> f2fs_reset_iostat(sbi);
>>> - 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);
>>> - }
>>> -
>>> return count;
>>> }
>>>
>>> @@ -349,8 +366,8 @@ F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent_sleep_time,
>>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_min_sleep_time, min_sleep_time);
>>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_max_sleep_time, max_sleep_time);
>>> F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_no_gc_sleep_time, no_gc_sleep_time);
>>
>> All above parameters will be accessed via '*ui = t;', in order to avoid that, we
>> need to move all gc_* configuration fields into sbi, so leaving f2fs_gc_kthread
>> all most empty.
>
> What I said was to use down_read(&sbi->sb->s_umount); in sysfs.c on top of this.
Oh, sorry, I didn't realize that, let me update my patch.
Thanks,
>
>> IMO, just merge my v1 patch is OK? since that patch makes better
>> encapsulation, so that background GC related parameter can still locate in
>> independent f2fs_gc_kthread structure rather in common f2fs_sb_info structure.
>>
>> Thanks,
>>
>>> -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_idle, gc_idle);
>>> -F2FS_RW_ATTR(GC_THREAD, f2fs_gc_kthread, gc_urgent, gc_urgent);
>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_idle, gc_mode);
>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode);
>>> F2FS_RW_ATTR(SM_INFO, f2fs_sm_info, reclaim_segments, rec_prefree_segments);
>>> F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, max_small_discards, max_discards);
>>> F2FS_RW_ATTR(DCC_INFO, discard_cmd_control, discard_granularity, discard_granularity);
>>>
>
> .
>
Powered by blists - more mailing lists