Re: [PATCH v2] ext4: move journal state away from orphan list updates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent sysfs writes to err_report_sec corrupt the timer list due to unnecessary timer_setup()
--

commit 5546001d405559aa5c5103006d23b436861ae874
Author: Jonas Zhou <jonaszhou@zhaoxin.com>
Subject: ext4: move journal state away from orphan list updates

This patch exchanges s_journal and s_ext4_flags with s_err_report_sec
and s_li_request in struct ext4_sb_info. This separates common journal
and flag readers from orphan-list writes to improve cache line usage
and avoid false sharing.

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 724a27e8be613..16ef010782331 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1641,9 +1641,13 @@ struct ext4_sb_info {
>  	struct super_block *s_sb;
>  	struct buffer_head *s_mmp_bh;
>  
> +	/* timeout in seconds for s_err_report; 0 disables the timer. */
> +	unsigned long s_err_report_sec;
> +

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the movement
of s_err_report_sec, I noticed a pre-existing issue in its sysfs store
handler.

Can concurrent writes to the err_report_sec sysfs file corrupt the timer list?

Looking at err_report_sec_store() in fs/ext4/sysfs.c, it does not hold any
locks. If two threads concurrently write non-zero values when the current
value is 0:

Thread A
  if (!sbi->s_err_report_sec && t)
      timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
  ...
  mod_timer(&sbi->s_err_report, ...);

Thread B
  if (!sbi->s_err_report_sec && t)
      timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
  ...
  mod_timer(&sbi->s_err_report, ...);

Thread A arms the timer and puts it in the active timer wheel. Thread B then
calls timer_setup() which zeroes timer->entry.pprev. This makes Thread B's
subsequent mod_timer() re-insert the timer without detaching it, corrupting
the timer wheel's doubly-linked hlist.

Since the timer is already unconditionally initialized during filesystem
mount, and timer_delete_sync() doesn't deinitialize it, is this timer_setup()
call in the sysfs handler unnecessary?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/a33183c1c24ea47a98afdd4756b49365593a2762.1788779199.git.jonaszhou@zhaoxin.com?part=1