[PATCH 1/4] f2fs: fix to avoid potential deadlock

[PATCH 1/4] f2fs: fix to avoid potential deadlock

[Chao Yu]

We should always check F2FS_I(inode)->cp_task condition in prior to other
conditions in __should_serialize_io() to avoid deadloop described in
commit 040d2bb318d1 ("f2fs: fix to avoid deadloop if data_flush is on"),
however we break this rule when we support compression, fix it.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index dbde309349d0..5c5db09324b7 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2962,15 +2962,17 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 static inline bool __should_serialize_io(struct inode *inode,
 					struct writeback_control *wbc)
 {
+	/* to avoid deadlock in path of data flush */
+	if (F2FS_I(inode)->cp_task)
+		return false;
+
 	if (!S_ISREG(inode->i_mode))
 		return false;
-	if (f2fs_compressed_file(inode))
-		return true;
 	if (IS_NOQUOTA(inode))
 		return false;
-	/* to avoid deadlock in path of data flush */
-	if (F2FS_I(inode)->cp_task)
-		return false;
+
+	if (f2fs_compressed_file(inode))
+		return true;
 	if (wbc->sync_mode != WB_SYNC_ALL)
 		return true;
 	if (get_dirty_pages(inode) >= SM_I(F2FS_I_SB(inode))->min_seq_blocks)
-- 
2.18.0.rc1

[PATCH 2/4] f2fs: don't trigger data flush in foreground operation

[Chao Yu]

Data flush can generate heavy IO and cause long latency during
flush, so it's not appropriate to trigger it in foreground
operation.

And also, we may face below potential deadlock during data flush:
- f2fs_write_multi_pages
 - f2fs_write_raw_pages
  - f2fs_write_single_data_page
   - f2fs_balance_fs
    - f2fs_balance_fs_bg
     - f2fs_sync_dirty_inodes
      - filemap_fdatawrite   -- stuck on flush same cluster

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/f2fs.h    | 2 +-
 fs/f2fs/gc.c      | 2 +-
 fs/f2fs/node.c    | 2 +-
 fs/f2fs/segment.c | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 422a070526e5..09db79a20f8e 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3246,7 +3246,7 @@ void f2fs_drop_inmem_pages(struct inode *inode);
 void f2fs_drop_inmem_page(struct inode *inode, struct page *page);
 int f2fs_commit_inmem_pages(struct inode *inode);
 void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need);
-void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi);
+void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi, bool from_bg);
 int f2fs_issue_flush(struct f2fs_sb_info *sbi, nid_t ino);
 int f2fs_create_flush_cmd_control(struct f2fs_sb_info *sbi);
 int f2fs_flush_device_cache(struct f2fs_sb_info *sbi);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index f122fe3dbba3..26248c8936db 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -113,7 +113,7 @@ static int gc_thread_func(void *data)
 				prefree_segments(sbi), free_segments(sbi));
 
 		/* balancing f2fs's metadata periodically */
-		f2fs_balance_fs_bg(sbi);
+		f2fs_balance_fs_bg(sbi, true);
 next:
 		sb_end_write(sbi->sb);
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 542335bdc100..ecbd6bd14a49 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1976,7 +1976,7 @@ static int f2fs_write_node_pages(struct address_space *mapping,
 		goto skip_write;
 
 	/* balancing f2fs's metadata in background */
-	f2fs_balance_fs_bg(sbi);
+	f2fs_balance_fs_bg(sbi, true);
 
 	/* collect a number of dirty node pages and write together */
 	if (wbc->sync_mode != WB_SYNC_ALL &&
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 601d67e72c50..aece09a184fa 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -496,7 +496,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
 
 	/* balance_fs_bg is able to be pending */
 	if (need && excess_cached_nats(sbi))
-		f2fs_balance_fs_bg(sbi);
+		f2fs_balance_fs_bg(sbi, false);
 
 	if (!f2fs_is_checkpoint_ready(sbi))
 		return;
@@ -511,7 +511,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
 	}
 }
 
-void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi)
+void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi, bool from_bg)
 {
 	if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
 		return;
@@ -540,7 +540,7 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi)
 			excess_dirty_nats(sbi) ||
 			excess_dirty_nodes(sbi) ||
 			f2fs_time_over(sbi, CP_TIME)) {
-		if (test_opt(sbi, DATA_FLUSH)) {
+		if (test_opt(sbi, DATA_FLUSH) && from_bg) {
 			struct blk_plug plug;
 
 			mutex_lock(&sbi->flush_lock);
-- 
2.18.0.rc1

[PATCH 3/4] f2fs: fix NULL pointer dereference in f2fs_verity_work()

[Chao Yu]

If both compression and fsverity feature is on, generic/572 will
report below NULL pointer dereference bug.

 BUG: kernel NULL pointer dereference, address: 0000000000000018
 RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
 #PF: supervisor read access in kernel mode
 Workqueue: fsverity_read_queue f2fs_verity_work [f2fs]
 RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
 Call Trace:
  process_one_work+0x16c/0x3f0
  worker_thread+0x4c/0x440
  ? rescuer_thread+0x350/0x350
  kthread+0xf8/0x130
  ? kthread_unpark+0x70/0x70
  ret_from_fork+0x35/0x40

There are two issue in f2fs_verity_work():
- it needs to traverse and verify all pages in bio.
- if pages in bio belong to non-compressed cluster, accessing
decompress IO context stored in page private will cause NULL
pointer dereference.

Fix them.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 5c5db09324b7..66e49fc1056e 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -187,12 +187,37 @@ static void f2fs_verify_pages(struct page **rpages, unsigned int cluster_size)
 
 static void f2fs_verify_bio(struct bio *bio)
 {
-	struct page *page = bio_first_page_all(bio);
-	struct decompress_io_ctx *dic =
-			(struct decompress_io_ctx *)page_private(page);
+	struct bio_vec *bv;
+	struct bvec_iter_all iter_all;
+	struct decompress_io_ctx *dic, *pdic = NULL;
+
+	bio_for_each_segment_all(bv, bio, iter_all) {
+		struct page *page = bv->bv_page;
+
+		dic = (struct decompress_io_ctx *)page_private(page);
+
+		if (dic) {
+			if (dic != pdic) {
+				f2fs_verify_pages(dic->rpages,
+							dic->cluster_size);
+				f2fs_free_dic(dic);
+				pdic = dic;
+			}
+			continue;
+		}
+		pdic = dic;
 
-	f2fs_verify_pages(dic->rpages, dic->cluster_size);
-	f2fs_free_dic(dic);
+		if (bio->bi_status || PageError(page)) {
+			ClearPageUptodate(page);
+			ClearPageError(page);
+		} else {
+			if (fsverity_verify_page(page))
+				SetPageUptodate(page);
+			else
+				SetPageError(page);
+		}
+		unlock_page(page);
+	}
 }
 #endif
 
-- 
2.18.0.rc1

[PATCH 4/4] f2fs: fix NULL pointer dereference in f2fs_write_begin()

[Chao Yu]

BUG: kernel NULL pointer dereference, address: 0000000000000000
RIP: 0010:f2fs_write_begin+0x823/0xb90 [f2fs]
Call Trace:
 f2fs_quota_write+0x139/0x1d0 [f2fs]
 write_blk+0x36/0x80 [quota_tree]
 get_free_dqblk+0x42/0xa0 [quota_tree]
 do_insert_tree+0x235/0x4a0 [quota_tree]
 do_insert_tree+0x26e/0x4a0 [quota_tree]
 do_insert_tree+0x26e/0x4a0 [quota_tree]
 do_insert_tree+0x26e/0x4a0 [quota_tree]
 qtree_write_dquot+0x70/0x190 [quota_tree]
 v2_write_dquot+0x43/0x90 [quota_v2]
 dquot_acquire+0x77/0x100
 f2fs_dquot_acquire+0x2f/0x60 [f2fs]
 dqget+0x310/0x450
 dquot_transfer+0x7e/0x120
 f2fs_setattr+0x11a/0x4a0 [f2fs]
 notify_change+0x349/0x480
 chown_common+0x168/0x1c0
 do_fchownat+0xbc/0xf0
 __x64_sys_fchownat+0x20/0x30
 do_syscall_64+0x5f/0x220
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Passing fsdata parameter to .write_{begin,end} in f2fs_quota_write(),
so that if quota file is compressed one, we can avoid above NULL
pointer dereference when updating quota content.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ebffe7aa08ee..b83b17b54a0a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1936,6 +1936,7 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
 	int offset = off & (sb->s_blocksize - 1);
 	size_t towrite = len;
 	struct page *page;
+	void *fsdata = NULL;
 	char *kaddr;
 	int err = 0;
 	int tocopy;
@@ -1945,7 +1946,7 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
 								towrite);
 retry:
 		err = a_ops->write_begin(NULL, mapping, off, tocopy, 0,
-							&page, NULL);
+							&page, &fsdata);
 		if (unlikely(err)) {
 			if (err == -ENOMEM) {
 				congestion_wait(BLK_RW_ASYNC,
@@ -1962,7 +1963,7 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
 		flush_dcache_page(page);
 
 		a_ops->write_end(NULL, mapping, off, tocopy, tocopy,
-						page, NULL);
+						page, fsdata);
 		offset = 0;
 		towrite -= tocopy;
 		off += tocopy;
-- 
2.18.0.rc1

Re: [PATCH 3/4] f2fs: fix NULL pointer dereference in f2fs_verity_work()

[Eric Biggers]

On Thu, Mar 19, 2020 at 07:57:59PM +0800, Chao Yu wrote:
> If both compression and fsverity feature is on, generic/572 will
> report below NULL pointer dereference bug.
> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000018
>  RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
>  #PF: supervisor read access in kernel mode
>  Workqueue: fsverity_read_queue f2fs_verity_work [f2fs]
>  RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
>  Call Trace:
>   process_one_work+0x16c/0x3f0
>   worker_thread+0x4c/0x440
>   ? rescuer_thread+0x350/0x350
>   kthread+0xf8/0x130
>   ? kthread_unpark+0x70/0x70
>   ret_from_fork+0x35/0x40
> 
> There are two issue in f2fs_verity_work():
> - it needs to traverse and verify all pages in bio.
> - if pages in bio belong to non-compressed cluster, accessing
> decompress IO context stored in page private will cause NULL
> pointer dereference.
> 
> Fix them.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/data.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 5c5db09324b7..66e49fc1056e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -187,12 +187,37 @@ static void f2fs_verify_pages(struct page **rpages, unsigned int cluster_size)
>  
>  static void f2fs_verify_bio(struct bio *bio)
>  {
> -	struct page *page = bio_first_page_all(bio);
> -	struct decompress_io_ctx *dic =
> -			(struct decompress_io_ctx *)page_private(page);
> +	struct bio_vec *bv;
> +	struct bvec_iter_all iter_all;
> +	struct decompress_io_ctx *dic, *pdic = NULL;
> +
> +	bio_for_each_segment_all(bv, bio, iter_all) {
> +		struct page *page = bv->bv_page;
> +
> +		dic = (struct decompress_io_ctx *)page_private(page);
> +
> +		if (dic) {
> +			if (dic != pdic) {
> +				f2fs_verify_pages(dic->rpages,
> +							dic->cluster_size);
> +				f2fs_free_dic(dic);
> +				pdic = dic;
> +			}
> +			continue;
> +		}
> +		pdic = dic;
>  
> -	f2fs_verify_pages(dic->rpages, dic->cluster_size);
> -	f2fs_free_dic(dic);
> +		if (bio->bi_status || PageError(page)) {
> +			ClearPageUptodate(page);
> +			ClearPageError(page);
> +		} else {
> +			if (fsverity_verify_page(page))
> +				SetPageUptodate(page);
> +			else
> +				SetPageError(page);
> +		}
> +		unlock_page(page);
> +	}

I'm a bit confused why you added SetPageError() before unlocking the page.
The other error paths actually clear the Error flag, not set it.  I thought
there's a reason for that?

- Eric

Re: [f2fs-dev] [PATCH 3/4] f2fs: fix NULL pointer dereference in f2fs_verity_work()

[Chao Yu]

On 2020-3-21 3:11, Eric Biggers wrote:
> On Thu, Mar 19, 2020 at 07:57:59PM +0800, Chao Yu wrote:
>> If both compression and fsverity feature is on, generic/572 will
>> report below NULL pointer dereference bug.
>>
>>  BUG: kernel NULL pointer dereference, address: 0000000000000018
>>  RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
>>  #PF: supervisor read access in kernel mode
>>  Workqueue: fsverity_read_queue f2fs_verity_work [f2fs]
>>  RIP: 0010:f2fs_verity_work+0x60/0x90 [f2fs]
>>  Call Trace:
>>   process_one_work+0x16c/0x3f0
>>   worker_thread+0x4c/0x440
>>   ? rescuer_thread+0x350/0x350
>>   kthread+0xf8/0x130
>>   ? kthread_unpark+0x70/0x70
>>   ret_from_fork+0x35/0x40
>>
>> There are two issue in f2fs_verity_work():
>> - it needs to traverse and verify all pages in bio.
>> - if pages in bio belong to non-compressed cluster, accessing
>> decompress IO context stored in page private will cause NULL
>> pointer dereference.
>>
>> Fix them.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/data.c | 35 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 5c5db09324b7..66e49fc1056e 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -187,12 +187,37 @@ static void f2fs_verify_pages(struct page **rpages, unsigned int cluster_size)
>>
>>  static void f2fs_verify_bio(struct bio *bio)
>>  {
>> -	struct page *page = bio_first_page_all(bio);
>> -	struct decompress_io_ctx *dic =
>> -			(struct decompress_io_ctx *)page_private(page);
>> +	struct bio_vec *bv;
>> +	struct bvec_iter_all iter_all;
>> +	struct decompress_io_ctx *dic, *pdic = NULL;
>> +
>> +	bio_for_each_segment_all(bv, bio, iter_all) {
>> +		struct page *page = bv->bv_page;
>> +
>> +		dic = (struct decompress_io_ctx *)page_private(page);
>> +
>> +		if (dic) {
>> +			if (dic != pdic) {
>> +				f2fs_verify_pages(dic->rpages,
>> +							dic->cluster_size);
>> +				f2fs_free_dic(dic);
>> +				pdic = dic;
>> +			}
>> +			continue;
>> +		}
>> +		pdic = dic;
>>
>> -	f2fs_verify_pages(dic->rpages, dic->cluster_size);
>> -	f2fs_free_dic(dic);
>> +		if (bio->bi_status || PageError(page)) {
>> +			ClearPageUptodate(page);
>> +			ClearPageError(page);
>> +		} else {
>> +			if (fsverity_verify_page(page))
>> +				SetPageUptodate(page);
>> +			else
>> +				SetPageError(page);
>> +		}
>> +		unlock_page(page);
>> +	}
>
> I'm a bit confused why you added SetPageError() before unlocking the page.
> The other error paths actually clear the Error flag, not set it.  I thought
> there's a reason for that?

These codes were copy&paste from f2fs_decompress_end_io(), I guess I missed to 
check that logic, so anyway, we need another patch to fix that first.

Thanks for pointing out this.

Thanks,

>
> - Eric
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>