Re: [f2fs-dev] [PATCH 5/5] f2fs: fix to avoid race during access gc_thread pointer

2018-04-20 Thread Chao Yu 
On 2018/4/20 11:54, Jaegeuk Kim wrote:
> On 04/20, Chao Yu wrote:
>> On 2018/4/20 11:19, Jaegeuk Kim wrote:
>>> On 04/18, Chao Yu wrote:
 Thread A   Thread BThread 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
>>>
>>> Do we simply need a lock for this?
>>
>> Code will be more complicated for handling existed and new coming fields with
>> the sbi->gc_thread pointer, and causing unneeded lock overhead, right?
>>
>> So let's just allocate memory during fill_super?
> 
> No, the case is when stopping the thread. We can keep the gc_thread and 
> indicate
> its state as "disabled". Then, we need to handle other paths with the state?

After this patch, we use f2fs_gc_kthread.f2fs_gc_task to indicate whether GC
thread is existed, so you mean if we do that change, we also need to add a lock,
and access/update other fields in f2fs_gc_kthread after we check
f2fs_gc_kthread.f2fs_gc_task with a lock, right?

Like:

f2fs_sbi_store:

if (!strcmp(a->attr.name, "gc_urgent") && t == 1) {
gc_context_lock()
if (GC_I(sbi)->f2fs_gc_task) {
GC_I(sbi)->gc_wake = 1;
...
}
gc_context_unlock()

Do you mean that?

Thanks,

> 
>>
>> Thanks,
>>
>>>

 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, keep gc_thread structure valid in sbi all
 the time instead of alloc/free it dynamically.

 Signed-off-by: Chao Yu 
 ---
  fs/f2fs/debug.c   |  3 +--
  fs/f2fs/f2fs.h|  7 +++
  fs/f2fs/gc.c  | 58 
 +--
  fs/f2fs/segment.c |  4 ++--
  fs/f2fs/super.c   | 13 +++--
  fs/f2fs/sysfs.c   |  8 
  6 files changed, 60 insertions(+), 33 deletions(-)

 diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
 index 715beb85e9db..7bb036a3bb81 100644
 --- a/fs/f2fs/debug.c
 +++ b/fs/f2fs/debug.c
 @@ -223,8 +223,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi)
si->cache_mem = 0;
  
/* build gc */
 -  if (sbi->gc_thread)
 -  si->cache_mem += sizeof(struct f2fs_gc_kthread);
 +  si->cache_mem += sizeof(struct f2fs_gc_kthread);
  
/* build merge flush thread */
if (SM_I(sbi)->fcc_info)
 diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
 index 567c6bb57ae3..c553f63199e8 100644
 --- a/fs/f2fs/f2fs.h
 +++ b/fs/f2fs/f2fs.h
 @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct 
 f2fs_sb_info *sbi)
return (struct sit_info *)(SM_I(sbi)->sit_info);
  }
  
 +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi)
 +{
 +  return (struct f2fs_gc_kthread *)(sbi->gc_thread);
 +}
 +
  static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi)
  {
return (struct free_segmap_info *)(SM_I(sbi)->free_info);
 @@ -2954,6 +2959,8 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t 
 pos, size_t len);
  /*
   * gc.c
   */
 +int init_gc_context(struct f2fs_sb_info *sbi);
 +void destroy_gc_context(struct f2fs_sb_info * sbi);
  int start_gc_thread(struct f2fs_sb_info *sbi);
  void stop_gc_thread(struct f2fs_sb_info *sbi);
  block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
 diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
 index da89ca16a55d..7d310e454b77 100644
 --- a/fs/f2fs/gc.c
 +++ b/fs/f2fs/gc.c
 @@ -26,8 +26,8 @@
  static int gc_thread_func(void *data)
  {
struct f2fs_sb_info *sbi = data;
 -  struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
 -  wait_queue_head_t *wq = >gc_thread->gc_wait_queue_head;
 +  struct f2fs_gc_kthread *gc_th = GC_I(sbi);
 +  wait_queue_head_t *wq = _th->gc_wait_queue_head;
unsigned int wait_ms;
  
wait_ms = gc_th->min_sleep_time;
 @@ -114,17 +114,15 @@ static int gc_thread_func(void *data)
return 0;
  }
  
 -int start_gc_thread(struct f2fs_sb_info *sbi)
 +int init_gc_context(struct f2fs_sb_info *sbi)
  {
struct f2fs_gc_kthread *gc_th;
 -  dev_t dev = sbi->sb->s_bdev->bd_dev;
 -  int err = 0;
  
gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL);
 -

Re: [f2fs-dev] [PATCH 5/5] f2fs: fix to avoid race during access gc_thread pointer

2018-04-19 Thread Jaegeuk Kim 
On 04/20, Chao Yu wrote:
> On 2018/4/20 11:19, Jaegeuk Kim wrote:
> > On 04/18, Chao Yu wrote:
> >> Thread A   Thread BThread 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
> > 
> > Do we simply need a lock for this?
> 
> Code will be more complicated for handling existed and new coming fields with
> the sbi->gc_thread pointer, and causing unneeded lock overhead, right?
> 
> So let's just allocate memory during fill_super?

No, the case is when stopping the thread. We can keep the gc_thread and indicate
its state as "disabled". Then, we need to handle other paths with the state?

> 
> Thanks,
> 
> > 
> >>
> >> 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, keep gc_thread structure valid in sbi all
> >> the time instead of alloc/free it dynamically.
> >>
> >> Signed-off-by: Chao Yu 
> >> ---
> >>  fs/f2fs/debug.c   |  3 +--
> >>  fs/f2fs/f2fs.h|  7 +++
> >>  fs/f2fs/gc.c  | 58 
> >> +--
> >>  fs/f2fs/segment.c |  4 ++--
> >>  fs/f2fs/super.c   | 13 +++--
> >>  fs/f2fs/sysfs.c   |  8 
> >>  6 files changed, 60 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> >> index 715beb85e9db..7bb036a3bb81 100644
> >> --- a/fs/f2fs/debug.c
> >> +++ b/fs/f2fs/debug.c
> >> @@ -223,8 +223,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi)
> >>si->cache_mem = 0;
> >>  
> >>/* build gc */
> >> -  if (sbi->gc_thread)
> >> -  si->cache_mem += sizeof(struct f2fs_gc_kthread);
> >> +  si->cache_mem += sizeof(struct f2fs_gc_kthread);
> >>  
> >>/* build merge flush thread */
> >>if (SM_I(sbi)->fcc_info)
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 567c6bb57ae3..c553f63199e8 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct 
> >> f2fs_sb_info *sbi)
> >>return (struct sit_info *)(SM_I(sbi)->sit_info);
> >>  }
> >>  
> >> +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi)
> >> +{
> >> +  return (struct f2fs_gc_kthread *)(sbi->gc_thread);
> >> +}
> >> +
> >>  static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi)
> >>  {
> >>return (struct free_segmap_info *)(SM_I(sbi)->free_info);
> >> @@ -2954,6 +2959,8 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t 
> >> pos, size_t len);
> >>  /*
> >>   * gc.c
> >>   */
> >> +int init_gc_context(struct f2fs_sb_info *sbi);
> >> +void destroy_gc_context(struct f2fs_sb_info * sbi);
> >>  int start_gc_thread(struct f2fs_sb_info *sbi);
> >>  void stop_gc_thread(struct f2fs_sb_info *sbi);
> >>  block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
> >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >> index da89ca16a55d..7d310e454b77 100644
> >> --- a/fs/f2fs/gc.c
> >> +++ b/fs/f2fs/gc.c
> >> @@ -26,8 +26,8 @@
> >>  static int gc_thread_func(void *data)
> >>  {
> >>struct f2fs_sb_info *sbi = data;
> >> -  struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
> >> -  wait_queue_head_t *wq = >gc_thread->gc_wait_queue_head;
> >> +  struct f2fs_gc_kthread *gc_th = GC_I(sbi);
> >> +  wait_queue_head_t *wq = _th->gc_wait_queue_head;
> >>unsigned int wait_ms;
> >>  
> >>wait_ms = gc_th->min_sleep_time;
> >> @@ -114,17 +114,15 @@ static int gc_thread_func(void *data)
> >>return 0;
> >>  }
> >>  
> >> -int start_gc_thread(struct f2fs_sb_info *sbi)
> >> +int init_gc_context(struct f2fs_sb_info *sbi)
> >>  {
> >>struct f2fs_gc_kthread *gc_th;
> >> -  dev_t dev = sbi->sb->s_bdev->bd_dev;
> >> -  int err = 0;
> >>  
> >>gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL);
> >> -  if (!gc_th) {
> >> -  err = -ENOMEM;
> >> -  goto out;
> >> -  }
> >> +  if (!gc_th)
> >> +  return -ENOMEM;
> >> +
> >> +  gc_th->f2fs_gc_task = NULL;
> >>  
> >>gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
> >>gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
> >> @@ -139,26 +137,41 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
> >>gc_th->atomic_file[FG_GC] = 0;
> >>  
> >>sbi->gc_thread = gc_th;
> >> -  init_waitqueue_head(>gc_thread->gc_wait_queue_head);
> >> -  sbi->gc_thread->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +void

Re: [f2fs-dev] [PATCH 5/5] f2fs: fix to avoid race during access gc_thread pointer

2018-04-19 Thread Chao Yu 
On 2018/4/20 11:19, Jaegeuk Kim wrote:
> On 04/18, Chao Yu wrote:
>> Thread A Thread BThread 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
> 
> Do we simply need a lock for this?

Code will be more complicated for handling existed and new coming fields with
the sbi->gc_thread pointer, and causing unneeded lock overhead, right?

So let's just allocate memory during fill_super?

Thanks,

> 
>>
>> 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, keep gc_thread structure valid in sbi all
>> the time instead of alloc/free it dynamically.
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fs/f2fs/debug.c   |  3 +--
>>  fs/f2fs/f2fs.h|  7 +++
>>  fs/f2fs/gc.c  | 58 
>> +--
>>  fs/f2fs/segment.c |  4 ++--
>>  fs/f2fs/super.c   | 13 +++--
>>  fs/f2fs/sysfs.c   |  8 
>>  6 files changed, 60 insertions(+), 33 deletions(-)
>>
>> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
>> index 715beb85e9db..7bb036a3bb81 100644
>> --- a/fs/f2fs/debug.c
>> +++ b/fs/f2fs/debug.c
>> @@ -223,8 +223,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi)
>>  si->cache_mem = 0;
>>  
>>  /* build gc */
>> -if (sbi->gc_thread)
>> -si->cache_mem += sizeof(struct f2fs_gc_kthread);
>> +si->cache_mem += sizeof(struct f2fs_gc_kthread);
>>  
>>  /* build merge flush thread */
>>  if (SM_I(sbi)->fcc_info)
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 567c6bb57ae3..c553f63199e8 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct 
>> f2fs_sb_info *sbi)
>>  return (struct sit_info *)(SM_I(sbi)->sit_info);
>>  }
>>  
>> +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi)
>> +{
>> +return (struct f2fs_gc_kthread *)(sbi->gc_thread);
>> +}
>> +
>>  static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi)
>>  {
>>  return (struct free_segmap_info *)(SM_I(sbi)->free_info);
>> @@ -2954,6 +2959,8 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t 
>> pos, size_t len);
>>  /*
>>   * gc.c
>>   */
>> +int init_gc_context(struct f2fs_sb_info *sbi);
>> +void destroy_gc_context(struct f2fs_sb_info * sbi);
>>  int start_gc_thread(struct f2fs_sb_info *sbi);
>>  void stop_gc_thread(struct f2fs_sb_info *sbi);
>>  block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index da89ca16a55d..7d310e454b77 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -26,8 +26,8 @@
>>  static int gc_thread_func(void *data)
>>  {
>>  struct f2fs_sb_info *sbi = data;
>> -struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
>> -wait_queue_head_t *wq = >gc_thread->gc_wait_queue_head;
>> +struct f2fs_gc_kthread *gc_th = GC_I(sbi);
>> +wait_queue_head_t *wq = _th->gc_wait_queue_head;
>>  unsigned int wait_ms;
>>  
>>  wait_ms = gc_th->min_sleep_time;
>> @@ -114,17 +114,15 @@ static int gc_thread_func(void *data)
>>  return 0;
>>  }
>>  
>> -int start_gc_thread(struct f2fs_sb_info *sbi)
>> +int init_gc_context(struct f2fs_sb_info *sbi)
>>  {
>>  struct f2fs_gc_kthread *gc_th;
>> -dev_t dev = sbi->sb->s_bdev->bd_dev;
>> -int err = 0;
>>  
>>  gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL);
>> -if (!gc_th) {
>> -err = -ENOMEM;
>> -goto out;
>> -}
>> +if (!gc_th)
>> +return -ENOMEM;
>> +
>> +gc_th->f2fs_gc_task = NULL;
>>  
>>  gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
>>  gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
>> @@ -139,26 +137,41 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
>>  gc_th->atomic_file[FG_GC] = 0;
>>  
>>  sbi->gc_thread = gc_th;
>> -init_waitqueue_head(>gc_thread->gc_wait_queue_head);
>> -sbi->gc_thread->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
>> +
>> +return 0;
>> +}
>> +
>> +void destroy_gc_context(struct f2fs_sb_info *sbi)
>> +{
>> +kfree(GC_I(sbi));
>> +sbi->gc_thread = NULL;
>> +}
>> +
>> +int start_gc_thread(struct f2fs_sb_info *sbi)
>> +{
>> +struct f2fs_gc_kthread *gc_th = GC_I(sbi);
>> +dev_t dev = sbi->sb->s_bdev->bd_dev;
>> +int err = 0;
>> +
>> +init_waitqueue_head(_th->gc_wait_queue_head);
>> +

Re: [f2fs-dev] [PATCH 5/5] f2fs: fix to avoid race during access gc_thread pointer

2018-04-19 Thread Jaegeuk Kim 
On 04/18, Chao Yu wrote:
> Thread A  Thread BThread 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

Do we simply need a lock for this?

> 
> 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, keep gc_thread structure valid in sbi all
> the time instead of alloc/free it dynamically.
> 
> Signed-off-by: Chao Yu 
> ---
>  fs/f2fs/debug.c   |  3 +--
>  fs/f2fs/f2fs.h|  7 +++
>  fs/f2fs/gc.c  | 58 
> +--
>  fs/f2fs/segment.c |  4 ++--
>  fs/f2fs/super.c   | 13 +++--
>  fs/f2fs/sysfs.c   |  8 
>  6 files changed, 60 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> index 715beb85e9db..7bb036a3bb81 100644
> --- a/fs/f2fs/debug.c
> +++ b/fs/f2fs/debug.c
> @@ -223,8 +223,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi)
>   si->cache_mem = 0;
>  
>   /* build gc */
> - if (sbi->gc_thread)
> - si->cache_mem += sizeof(struct f2fs_gc_kthread);
> + si->cache_mem += sizeof(struct f2fs_gc_kthread);
>  
>   /* build merge flush thread */
>   if (SM_I(sbi)->fcc_info)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 567c6bb57ae3..c553f63199e8 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct 
> f2fs_sb_info *sbi)
>   return (struct sit_info *)(SM_I(sbi)->sit_info);
>  }
>  
> +static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi)
> +{
> + return (struct f2fs_gc_kthread *)(sbi->gc_thread);
> +}
> +
>  static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi)
>  {
>   return (struct free_segmap_info *)(SM_I(sbi)->free_info);
> @@ -2954,6 +2959,8 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t pos, 
> size_t len);
>  /*
>   * gc.c
>   */
> +int init_gc_context(struct f2fs_sb_info *sbi);
> +void destroy_gc_context(struct f2fs_sb_info * sbi);
>  int start_gc_thread(struct f2fs_sb_info *sbi);
>  void stop_gc_thread(struct f2fs_sb_info *sbi);
>  block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index da89ca16a55d..7d310e454b77 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -26,8 +26,8 @@
>  static int gc_thread_func(void *data)
>  {
>   struct f2fs_sb_info *sbi = data;
> - struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
> - wait_queue_head_t *wq = >gc_thread->gc_wait_queue_head;
> + struct f2fs_gc_kthread *gc_th = GC_I(sbi);
> + wait_queue_head_t *wq = _th->gc_wait_queue_head;
>   unsigned int wait_ms;
>  
>   wait_ms = gc_th->min_sleep_time;
> @@ -114,17 +114,15 @@ static int gc_thread_func(void *data)
>   return 0;
>  }
>  
> -int start_gc_thread(struct f2fs_sb_info *sbi)
> +int init_gc_context(struct f2fs_sb_info *sbi)
>  {
>   struct f2fs_gc_kthread *gc_th;
> - dev_t dev = sbi->sb->s_bdev->bd_dev;
> - int err = 0;
>  
>   gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL);
> - if (!gc_th) {
> - err = -ENOMEM;
> - goto out;
> - }
> + if (!gc_th)
> + return -ENOMEM;
> +
> + gc_th->f2fs_gc_task = NULL;
>  
>   gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
>   gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
> @@ -139,26 +137,41 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
>   gc_th->atomic_file[FG_GC] = 0;
>  
>   sbi->gc_thread = gc_th;
> - init_waitqueue_head(>gc_thread->gc_wait_queue_head);
> - sbi->gc_thread->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
> +
> + return 0;
> +}
> +
> +void destroy_gc_context(struct f2fs_sb_info *sbi)
> +{
> + kfree(GC_I(sbi));
> + sbi->gc_thread = NULL;
> +}
> +
> +int start_gc_thread(struct f2fs_sb_info *sbi)
> +{
> + struct f2fs_gc_kthread *gc_th = GC_I(sbi);
> + dev_t dev = sbi->sb->s_bdev->bd_dev;
> + int err = 0;
> +
> + init_waitqueue_head(_th->gc_wait_queue_head);
> + gc_th->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
>   "f2fs_gc-%u:%u", MAJOR(dev), MINOR(dev));
>   if (IS_ERR(gc_th->f2fs_gc_task)) {
>   err = PTR_ERR(gc_th->f2fs_gc_task);
> - kfree(gc_th);
> - sbi->gc_thread = NULL;
> + gc_th->f2fs_gc_task = NULL;
>   }
> -out:
> +
>

[f2fs-dev] [PATCH 5/5] f2fs: fix to avoid race during access gc_thread pointer

2018-04-18 Thread Chao Yu 
Thread AThread BThread 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, keep gc_thread structure valid in sbi all
the time instead of alloc/free it dynamically.

Signed-off-by: Chao Yu 
---
 fs/f2fs/debug.c   |  3 +--
 fs/f2fs/f2fs.h|  7 +++
 fs/f2fs/gc.c  | 58 +--
 fs/f2fs/segment.c |  4 ++--
 fs/f2fs/super.c   | 13 +++--
 fs/f2fs/sysfs.c   |  8 
 6 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index 715beb85e9db..7bb036a3bb81 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -223,8 +223,7 @@ static void update_mem_info(struct f2fs_sb_info *sbi)
si->cache_mem = 0;
 
/* build gc */
-   if (sbi->gc_thread)
-   si->cache_mem += sizeof(struct f2fs_gc_kthread);
+   si->cache_mem += sizeof(struct f2fs_gc_kthread);
 
/* build merge flush thread */
if (SM_I(sbi)->fcc_info)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 567c6bb57ae3..c553f63199e8 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1412,6 +1412,11 @@ static inline struct sit_info *SIT_I(struct f2fs_sb_info 
*sbi)
return (struct sit_info *)(SM_I(sbi)->sit_info);
 }
 
+static inline struct f2fs_gc_kthread *GC_I(struct f2fs_sb_info *sbi)
+{
+   return (struct f2fs_gc_kthread *)(sbi->gc_thread);
+}
+
 static inline struct free_segmap_info *FREE_I(struct f2fs_sb_info *sbi)
 {
return (struct free_segmap_info *)(SM_I(sbi)->free_info);
@@ -2954,6 +2959,8 @@ bool f2fs_overwrite_io(struct inode *inode, loff_t pos, 
size_t len);
 /*
  * gc.c
  */
+int init_gc_context(struct f2fs_sb_info *sbi);
+void destroy_gc_context(struct f2fs_sb_info * sbi);
 int start_gc_thread(struct f2fs_sb_info *sbi);
 void stop_gc_thread(struct f2fs_sb_info *sbi);
 block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index da89ca16a55d..7d310e454b77 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -26,8 +26,8 @@
 static int gc_thread_func(void *data)
 {
struct f2fs_sb_info *sbi = data;
-   struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
-   wait_queue_head_t *wq = >gc_thread->gc_wait_queue_head;
+   struct f2fs_gc_kthread *gc_th = GC_I(sbi);
+   wait_queue_head_t *wq = _th->gc_wait_queue_head;
unsigned int wait_ms;
 
wait_ms = gc_th->min_sleep_time;
@@ -114,17 +114,15 @@ static int gc_thread_func(void *data)
return 0;
 }
 
-int start_gc_thread(struct f2fs_sb_info *sbi)
+int init_gc_context(struct f2fs_sb_info *sbi)
 {
struct f2fs_gc_kthread *gc_th;
-   dev_t dev = sbi->sb->s_bdev->bd_dev;
-   int err = 0;
 
gc_th = f2fs_kmalloc(sbi, sizeof(struct f2fs_gc_kthread), GFP_KERNEL);
-   if (!gc_th) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!gc_th)
+   return -ENOMEM;
+
+   gc_th->f2fs_gc_task = NULL;
 
gc_th->urgent_sleep_time = DEF_GC_THREAD_URGENT_SLEEP_TIME;
gc_th->min_sleep_time = DEF_GC_THREAD_MIN_SLEEP_TIME;
@@ -139,26 +137,41 @@ int start_gc_thread(struct f2fs_sb_info *sbi)
gc_th->atomic_file[FG_GC] = 0;
 
sbi->gc_thread = gc_th;
-   init_waitqueue_head(>gc_thread->gc_wait_queue_head);
-   sbi->gc_thread->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
+
+   return 0;
+}
+
+void destroy_gc_context(struct f2fs_sb_info *sbi)
+{
+   kfree(GC_I(sbi));
+   sbi->gc_thread = NULL;
+}
+
+int start_gc_thread(struct f2fs_sb_info *sbi)
+{
+   struct f2fs_gc_kthread *gc_th = GC_I(sbi);
+   dev_t dev = sbi->sb->s_bdev->bd_dev;
+   int err = 0;
+
+   init_waitqueue_head(_th->gc_wait_queue_head);
+   gc_th->f2fs_gc_task = kthread_run(gc_thread_func, sbi,
"f2fs_gc-%u:%u", MAJOR(dev), MINOR(dev));
if (IS_ERR(gc_th->f2fs_gc_task)) {
err = PTR_ERR(gc_th->f2fs_gc_task);
-   kfree(gc_th);
-   sbi->gc_thread = NULL;
+   gc_th->f2fs_gc_task = NULL;
}
-out:
+
return err;
 }
 
 void stop_gc_thread(struct f2fs_sb_info *sbi)
 {
-   struct f2fs_gc_kthread *gc_th = sbi->gc_thread;
-   if (!gc_th)
-   return;
-   kthread_stop(gc_th->f2fs_gc_task);
-   kfree(gc_th);