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 <yuch...@huawei.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 <jaeg...@kernel.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 <jaeg...@kernel.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);

F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, gc_urgent, gc_mode);

We have moved gc_urgent into F2FS_SBI set, but still we will try to access
gc_wait_queue_head queue head after verifying sbi->gc_thread pointer.

So we need to add s_umount here to avoid racing with remount?

Thanks,

> +                             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);
> -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);
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to