[lvc-project] [PATCH] gfs2: Remove unnecessary sd_jdesc NULL check in gfs2_log_flush

[Fedor Pchelkin]

On Tue, 14. Apr 01:34, Nikolai Kuznetsov wrote:
> Previous commit added an explicit NULL check for sd_jdesc to avoid

При чтении не ясно, что за предыдущий коммит.  Лучше написать явно

Commit 35264909e9d1 ("gfs2: Fix NULL pointer dereference in
gfs2_log_flush") added ...

> a race with unmount. However, in gfs2_log_flush() already goto out
> when SDF_JOURNAL_LIVE is cleared, which happens during the final
> shutdown flush before sd_jdesc is set to NULL. The explicit check
> is redundant, remove it.

Формулировка "in gfs2_log_flush() already goto out when SDF_JOURNAL_LIVE
is cleared" не соотносится с грамматикой англ. языка.  Можно написать:
gfs2_log_flush() already checks for SDF_JOURNAL_LIVE bit and bails out if
it's not set.  During unmount this bit is cleared in ... and the
filesystem becomes RO firstly and only then sd_jdesc is assigned NULL in
...  So the race is not possible.

Помимо того, что проверка redundant, в описании стоит явно упомянуть, что
указатель может быть разыменован дальше в функции gfs2_log_flush()
например там-то и там-то.  Собственно на это и ругается стат. анализатор.
Иначе непонятно, зачем предлагаете убирать проверку.  Ну есть она и есть
- зачем её убирать патчем?  Сейчас описание довольно скупо.

Также вы можете упомянуть в описании, что проблема, адресуемая виновным
коммитом, не является действительной по таким-то причинам.  Это ведь так?

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 35264909e9d1 ("gfs2: Fix NULL pointer dereference in gfs2_log_flush")
> Cc: stable at vger.kernel.org

Строчку Cc:stable нужно убрать.  Патч удаляет лишнюю проверку - это
улучшение читаемости, логики, последовательности кода и т.д. и т.п., но не
фикс реальной ошибки или падения.

Fixes можно оставить, это на усмотрение сопровождающего драйвера в данном
случае.

> Signed-off-by: Nikolai Kuznetsov <niku.csmsu at yandex.ru> 
> ---

Ещё, т.к. патч готовится под отправку в апстрим межд. сообщества, его
стоит готовить поверх [1].  В идеале поверх [2].  Кстати, посмотрите на
последний коммит [3].  Это ветка сопровождающего файловой системы, в
которой он накапливает изменения для последующей отправки в апстрим.
Похоже там на эту ситуацию всё ещё смотрят как на проблемную.

Своим патчем и, главное, доводами из описания патча (поэтому его важно
правильно составить), спровоцируете повторный анализ проблемы на его
стороне.  Он либо согласится с нашей логикой, либо укажет на то, что
упускаем.

Лучше подготовить патч поверх [2], и заодно убрать проверку из
gfs2_log_release().  Просьба его направить также пока только на
lvc-project.

Чтобы не клонить к себе репозиторий с ядром ещё раз, можете провернуть
следующее изнутри того, который себе уже склонировали ('-C $CLONE_DIR'
можно опустить если уже находитесь внутри корня репозитория):

git -C $CLONE_DIR remote add gfs2-origin https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/
git -C $CLONE_DIR fetch gfs2-origin
git -C $CLONE_DIR checkout --track gfs2-origin/for-next

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=for-next
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?h=for-next&id=74b4dbb946060a3233604d91859a9abd3708141d

>  fs/gfs2/log.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 347df29d610e..e4457fe6d9d6 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -1096,8 +1096,9 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl, u32 flags)
>  	lops_before_commit(sdp, tr);
>  	if (gfs2_withdrawn(sdp))
>  		goto out_withdraw;
> -	if (sdp->sd_jdesc)
> -		gfs2_log_submit_write(&sdp->sd_jdesc->jd_log_bio);
> +
> +	gfs2_log_submit_write(&sdp->sd_jdesc->jd_log_bio);
> +
>  	if (gfs2_withdrawn(sdp))
>  		goto out_withdraw;
>  
> -- 
> 2.43.0