[lvc-project] [PATCH] f2fs: Add a flush_work() call before destroying raw_super

[Fedor Pchelkin]

On Tue, 05. May 17:19, aitsygunka at yandex.ru wrote:
> Ага, конкретно сюда:
> 	/* get an inode for meta space */
> 	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
> 	if (IS_ERR(sbi->meta_inode)) {
> 		f2fs_err(sbi, "Failed to read F2FS meta data inode");
> 		err = PTR_ERR(sbi->meta_inode);
> 		// <-- вот сюда, т.к. именно отсюда мы переходим
> 		goto free_page_array_cache;
> 	}

Такое точно не годится, т.к. подавляет только конкретный репродьюсер с
конкретной трассой, а сама проблема может например возникнуть при
обработке ошибки следующей функции

	err = f2fs_get_valid_checkpoint(sbi);
	if (err) {
		f2fs_err(sbi, "Failed to get valid F2FS checkpoint");
		goto free_meta_inode;
	}

и т.д.

>  
> > Ну, я имел ввиду перенос уже имеющегося внутри f2fs_fill_super() вызова
> > flush_work() на пути обработки ошибки. Верно ли он поставлен? Может
> > быть, его лучше переместить в другое место, ниже по коду?
> 
> Да, я понял, оно логично, но у меня нет четкого понимания, что при переходе на метки
> выше мы также не получим состояние гонки.

По-хорошему порядок вызова функций на пути обработки ошибки в
f2fs_fill_super() должен соответствовать таковому в f2fs_put_super().
Но тут похоже порядок в f2fs_fill_super перемудрён.  Можно попробовать
начать с дублирования вызова flush_work(), но главное - отразить в
описании патча мотивы его _дополнительной_ постановки.  Должно быть ясно,
что вы проанализировали этот момент с дублированием и всё-таки решили
остановиться на нём, а не на перемещении вызова flush_work, вследствие
таких-то причин.

На мой взгляд перемещение вызова flush_work

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ccf806b676f5..75215dc78262 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -5418,7 +5418,6 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
 stop_ckpt_thread:
	f2fs_stop_ckpt_thread(sbi);
	/* flush s_error_work before sbi destroy */
-	flush_work(&sbi->s_error_work);
	f2fs_destroy_post_read_wq(sbi);
 free_devices:
	destroy_device_list(sbi);
@@ -5449,6 +5448,7 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
	/* no need to free dummy_enc_policy, we just keep it in ctx when failed */
	swap(F2FS_CTX_INFO(ctx).dummy_enc_policy, F2FS_OPTION(sbi).dummy_enc_policy);
 free_sb_buf:
+	flush_work(&sbi->s_error_work);
	kfree(raw_super);
 free_sbi:
 #ifdef CONFIG_DEBUG_LOCK_ALLOC

вполне может годиться.  Не вижу, чтобы воркер-обработчик s_error_work
касался полей, освобождаемых до метки free_sb_buf, т.е use-after-free быть
не должно.  Но здесь уже мэйнтенеры смогут более точно соориентировать.