LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
@ 2018-05-07 12:26 Chao Yu
  2018-05-07 21:36 ` Jaegeuk Kim
  0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2018-05-07 12:26 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, introduce gc_rwsem to exclude those operations.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
v4:
- use introduced sbi.gc_rwsem lock instead of sb.s_umount.
 fs/f2fs/f2fs.h    |  1 +
 fs/f2fs/gc.c      |  4 ++++
 fs/f2fs/segment.c | 11 ++++++++++-
 fs/f2fs/super.c   | 19 ++++++++++++++-----
 fs/f2fs/sysfs.c   | 14 +++++++++++---
 5 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 80490a7991a7..e238d0ea0be7 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1191,6 +1191,7 @@ struct f2fs_sb_info {
 
 	/* for cleaning operations */
 	struct mutex gc_mutex;			/* mutex for GC */
+	struct rw_semaphore gc_rwsem;		/* rw semaphore for gc_thread */
 	struct f2fs_gc_kthread	*gc_thread;	/* GC thread */
 	unsigned int cur_victim_sec;		/* current victim section num */
 
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 9bb2ddbbed1e..b74714be7be7 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->gc_rwsem);
 		p->gc_mode = select_gc_type(sbi->gc_thread, gc_type);
+		up_read(&sbi->gc_rwsem);
 		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->gc_rwsem);
 	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->gc_rwsem);
 
 	/* 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..33d146939048 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->gc_rwsem);
 	if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
-		return true;
+		gc_urgent = true;
+	up_read(&sbi->gc_rwsem);
+
+	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->gc_rwsem);
 		if (sbi->gc_thread && sbi->gc_thread->gc_urgent)
 			init_discard_policy(&dpolicy, DPOLICY_FORCE, 1);
+		up_read(&sbi->gc_rwsem);
 
 		sb_start_intwrite(sbi->sb);
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index c8e5fe5d71fe..294be9e92aee 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1476,15 +1476,23 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
 	 * option. Also sync the filesystem.
 	 */
 	if ((*flags & SB_RDONLY) || !test_opt(sbi, BG_GC)) {
+		down_write(&sbi->gc_rwsem);
 		if (sbi->gc_thread) {
 			stop_gc_thread(sbi);
 			need_restart_gc = true;
 		}
-	} else if (!sbi->gc_thread) {
-		err = start_gc_thread(sbi);
-		if (err)
-			goto restore_opts;
-		need_stop_gc = true;
+		up_write(&sbi->gc_rwsem);
+	} else {
+		down_write(&sbi->gc_rwsem);
+		if (!sbi->gc_thread) {
+			err = start_gc_thread(sbi);
+			if (err) {
+				up_write(&sbi->gc_rwsem);
+				goto restore_opts;
+			}
+			need_stop_gc = true;
+		}
+		up_write(&sbi->gc_rwsem);
 	}
 
 	if (*flags & SB_RDONLY ||
@@ -2802,6 +2810,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	init_rwsem(&sbi->cp_rwsem);
+	init_rwsem(&sbi->gc_rwsem);
 	init_waitqueue_head(&sbi->cp_wait);
 	init_sb_info(sbi);
 
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 6d8d8f41e517..34c542dbc459 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->gc_rwsem);
+
 	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->gc_rwsem);
+
 	return count;
 }
 
-- 
2.17.0.391.g1f1cddd558b5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
  2018-05-07 12:26 [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer Chao Yu
@ 2018-05-07 21:36 ` Jaegeuk Kim
  2018-05-08  1:56   ` Chao Yu
  2018-05-18  1:51   ` Chao Yu
  0 siblings, 2 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2018-05-07 21:36 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao

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@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 <jaegeuk@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 <jaegeuk@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);
+				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);
-- 
2.17.0.484.g0c8726318c-goog

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
  2018-05-07 21:36 ` Jaegeuk Kim
@ 2018-05-08  1:56   ` Chao Yu
  2018-05-08  3:17     ` Jaegeuk Kim
  2018-05-18  1:51   ` Chao Yu
  1 sibling, 1 reply; 6+ messages in thread
From: Chao Yu @ 2018-05-08  1:56 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao

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@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 <jaegeuk@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 <jaegeuk@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);
> +				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. 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);
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
  2018-05-08  1:56   ` Chao Yu
@ 2018-05-08  3:17     ` Jaegeuk Kim
  2018-05-08  3:21       ` Chao Yu
  0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2018-05-08  3:17 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, chao

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@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 <jaegeuk@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 <jaegeuk@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);
> > +				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.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
  2018-05-08  3:17     ` Jaegeuk Kim
@ 2018-05-08  3:21       ` Chao Yu
  0 siblings, 0 replies; 6+ messages in thread
From: Chao Yu @ 2018-05-08  3:21 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao

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@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 <jaegeuk@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 <jaegeuk@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);
>>> +				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);
>>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer
  2018-05-07 21:36 ` Jaegeuk Kim
  2018-05-08  1:56   ` Chao Yu
@ 2018-05-18  1:51   ` Chao Yu
  1 sibling, 0 replies; 6+ messages in thread
From: Chao Yu @ 2018-05-18  1:51 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao

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@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 <jaegeuk@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 <jaegeuk@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);
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-18  1:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-07 12:26 [PATCH v4] f2fs: fix to avoid race during access gc_thread pointer Chao Yu
2018-05-07 21:36 ` Jaegeuk Kim
2018-05-08  1:56   ` Chao Yu
2018-05-08  3:17     ` Jaegeuk Kim
2018-05-08  3:21       ` Chao Yu
2018-05-18  1:51   ` 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).