2020-03-18 06:13:41

by Ritesh Harjani

Subject: [PATCH] ext4: Unregister sysfs path before destroying jbd2 journal

Call ext4_unregister_sysfs(), before destroying jbd2 journal,
since below might cause, NULL pointer dereference issue.
This got reported with LTP tests.

ext4_put_super() cat /sys/fs/ext4/loop2/journal_task
| ext4_attr_show();
ext4_jbd2_journal_destroy(); |
| journal_task_show()
| |
| task_pid_vnr(NULL);
sbi->s_journal = NULL;

Signed-off-by: Ritesh Harjani <[email protected]>
---
fs/ext4/super.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 5dc65b7583cb..27ab130a40d1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1024,6 +1024,13 @@ static void ext4_put_super(struct super_block *sb)

destroy_workqueue(sbi->rsv_conversion_wq);

+ /*
+ * Unregister sysfs before destroying jbd2 journal.
+ * Since we could still access attr_journal_task attribute via sysfs
+ * path which could have sbi->s_journal->j_task as NULL
+ */
+ ext4_unregister_sysfs(sb);
+
if (sbi->s_journal) {
aborted = is_journal_aborted(sbi->s_journal);
err = jbd2_journal_destroy(sbi->s_journal);
@@ -1034,7 +1041,6 @@ static void ext4_put_super(struct super_block *sb)
}
}

- ext4_unregister_sysfs(sb);
ext4_es_unregister_shrinker(sbi);
del_timer_sync(&sbi->s_err_report);
ext4_release_system_zone(sb);
--
2.21.0

2020-03-18 09:13:47

by Jan Kara

Subject: Re: [PATCH] ext4: Unregister sysfs path before destroying jbd2 journal

On Wed 18-03-20 11:43:01, Ritesh Harjani wrote:
> Call ext4_unregister_sysfs(), before destroying jbd2 journal,
> since below might cause, NULL pointer dereference issue.
> This got reported with LTP tests.
>
> ext4_put_super() cat /sys/fs/ext4/loop2/journal_task
> | ext4_attr_show();
> ext4_jbd2_journal_destroy(); |
> | journal_task_show()
> | |
> | task_pid_vnr(NULL);
> sbi->s_journal = NULL;
>
> Signed-off-by: Ritesh Harjani <[email protected]>

Yeah, makes sence. Thanks for the patch! You can add:

Reviewed-by: Jan Kara <[email protected]>

Honza

> ---
> fs/ext4/super.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 5dc65b7583cb..27ab130a40d1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1024,6 +1024,13 @@ static void ext4_put_super(struct super_block *sb)
>
> destroy_workqueue(sbi->rsv_conversion_wq);
>
> + /*
> + * Unregister sysfs before destroying jbd2 journal.
> + * Since we could still access attr_journal_task attribute via sysfs
> + * path which could have sbi->s_journal->j_task as NULL
> + */
> + ext4_unregister_sysfs(sb);
> +
> if (sbi->s_journal) {
> aborted = is_journal_aborted(sbi->s_journal);
> err = jbd2_journal_destroy(sbi->s_journal);
> @@ -1034,7 +1041,6 @@ static void ext4_put_super(struct super_block *sb)
> }
> }
>
> - ext4_unregister_sysfs(sb);
> ext4_es_unregister_shrinker(sbi);
> del_timer_sync(&sbi->s_err_report);
> ext4_release_system_zone(sb);
> --
> 2.21.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2020-03-26 14:53:57

by Theodore Ts'o

Subject: Re: [PATCH] ext4: Unregister sysfs path before destroying jbd2 journal

On Wed, Mar 18, 2020 at 10:13:18AM +0100, Jan Kara wrote:
> On Wed 18-03-20 11:43:01, Ritesh Harjani wrote:
> > Call ext4_unregister_sysfs(), before destroying jbd2 journal,
> > since below might cause, NULL pointer dereference issue.
> > This got reported with LTP tests.
> >
> > ext4_put_super() cat /sys/fs/ext4/loop2/journal_task
> > | ext4_attr_show();
> > ext4_jbd2_journal_destroy(); |
> > | journal_task_show()
> > | |
> > | task_pid_vnr(NULL);
> > sbi->s_journal = NULL;
> >
> > Signed-off-by: Ritesh Harjani <[email protected]>
>
> Yeah, makes sence. Thanks for the patch! You can add:
>
> Reviewed-by: Jan Kara <[email protected]>

Applied, thanks.

- Ted