Ye Bin (2):
ext4: Fix use-after-free about sbi->s_mmp_tsk
ext4: Fix potential uas-after-free about sbi->s_mmp_tsk when kmmpd
kthread exit before set sbi->s_mmp_tsk
fs/ext4/ext4.h | 1 +
fs/ext4/mmp.c | 34 ++++++++++++++++++++++++++++------
fs/ext4/super.c | 1 +
3 files changed, 30 insertions(+), 6 deletions(-)
--
2.31.1
Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
already running and even exit as exception. Even though we set
sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
"sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
mount kmmpd
| |
|-call kthread_run |
| |-kmmpd runing
| |-kmmpd exit sbi->s_mmp_tsk=NULL
| |
|-kthread_run return |
| and set sbi->s_mmp_tsk |
| |
|-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF
This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
NULL. We can create kthread first, and then wakeup kmmpd kthread later.
Signed-off-by: Ye Bin <[email protected]>
---
fs/ext4/mmp.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index fc18a8c205c7..6ec1ea182cc0 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -394,16 +394,18 @@ int ext4_multi_mount_protect(struct super_block *sb,
/*
* Start a kernel thread to update the MMP block periodically.
*/
- EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, sb, "kmmpd-%.*s",
- (int)sizeof(mmp->mmp_bdevname),
- bdevname(bh->b_bdev,
- mmp->mmp_bdevname));
+ EXT4_SB(sb)->s_mmp_tsk = kthread_create(kmmpd, sb, "kmmpd-%.*s",
+ (int)sizeof(mmp->mmp_bdevname),
+ bdevname(bh->b_bdev,
+ mmp->mmp_bdevname));
+
if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
EXT4_SB(sb)->s_mmp_tsk = NULL;
ext4_warning(sb, "Unable to create kmmpd thread for %s.",
sb->s_id);
goto failed;
}
+ wake_up_process(EXT4_SB(sb)->s_mmp_tsk);
return 0;
--
2.31.1
After merge 618f003199c6("ext4: fix memory leak in ext4_fill_super") commit,
we add delay in ext4_remount after "sb->s_flags |= SB_RDONLY", then
remount filesystem with read-only kasan report following warning:
[ 888.695326] ==================================================================
[ 888.696566] BUG: KASAN: use-after-free in kthread_stop+0x4c/0x2f0
[ 888.697599] Write of size 4 at addr ffff8883849e0020 by task mount/2013
[ 888.698707]
[ 888.698982] CPU: 4 PID: 2013 Comm: mount Not tainted 4.19.95-00013-ga369a6189bbf-dirty #413
[ 888.700376] Hardware name: QEMU Standard PC
[ 888.702587] Call Trace:
[ 888.703017] dump_stack+0x108/0x15f
[ 888.703615] print_address_description+0xa5/0x372
[ 888.704420] kasan_report.cold+0x236/0x2a8
[ 888.705761] check_memory_region+0x240/0x270
[ 888.706486] kasan_check_write+0x20/0x30
[ 888.707156] kthread_stop+0x4c/0x2f0
[ 888.707776] ext4_stop_mmpd+0x32/0x90
[ 888.708262] ext4_remount.cold+0xf6/0x116
[ 888.712671] do_remount_sb+0xff/0x460
[ 888.714007] do_mount+0xce3/0x1be0
[ 888.717749] ksys_mount+0xb2/0x150
[ 888.718163] __x64_sys_mount+0x6a/0x80
[ 888.718607] do_syscall_64+0xd9/0x1f0
[ 888.719047] entry_SYSCALL_64_after_hwframe+0x44/0xa9
As kmmpd will exit if filesystem is read-only. Then sbi->s_mmp_tsk become wild
ptr, lead to use-after-free. As kmmpd will exit by others(call ktread_stop)
or by itself. After 618f003199c6 commit we can trigger this issue very easy.
Before this commit also exist this issue.
If kmmpd exit by itself, after merge 618f003199c6 commit there will trigger UAF
when umount filesystem.
To fix this issue, introduce sbi->s_mmp_lock to protect sbi->s_mmp_tsk. If kmmpd
exit by itself, we set sbi->s_mmp_tsk with NULL, and release mmp buffer_head.
Fixes: 618f003199c6 ("ext4: fix memory leak in ext4_fill_super")
Signed-off-by: Ye Bin <[email protected]>
---
fs/ext4/ext4.h | 1 +
fs/ext4/mmp.c | 24 ++++++++++++++++++++++--
fs/ext4/super.c | 1 +
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d3446746718..a479da37fed4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1489,6 +1489,7 @@ struct ext4_sb_info {
struct completion s_kobj_unregister;
struct super_block *s_sb;
struct buffer_head *s_mmp_bh;
+ struct mutex s_mmp_lock;
/* Journaling */
struct journal_s *s_journal;
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..fc18a8c205c7 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -128,8 +128,9 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
static int kmmpd(void *data)
{
struct super_block *sb = (struct super_block *) data;
- struct ext4_super_block *es = EXT4_SB(sb)->s_es;
- struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
+ struct ext4_sb_info *sbi = EXT4_SB(sb);
+ struct ext4_super_block *es = sbi->s_es;
+ struct buffer_head *bh = sbi->s_mmp_bh;
struct mmp_struct *mmp;
ext4_fsblk_t mmp_block;
u32 seq = 0;
@@ -245,16 +246,35 @@ static int kmmpd(void *data)
retval = write_mmp_block(sb, bh);
exit_thread:
+ /*
+ * Maybe s_mmp_tsk kthread is stoped by others or by itself. If exit
+ * by itself then sbi->s_mmp_tsk will be wild ptr, so there is need
+ * set sbi->s_mmp_tsk with NULL, and also release mmp buffer_head.
+ */
+ while (!kthread_should_stop()) {
+ if (!mutex_trylock(&sbi->s_mmp_lock))
+ continue;
+
+ if (sbi->s_mmp_tsk) {
+ sbi->s_mmp_tsk = NULL;
+ brelse(bh);
+ }
+ mutex_unlock(&sbi->s_mmp_lock);
+ break;
+ }
+
return retval;
}
void ext4_stop_mmpd(struct ext4_sb_info *sbi)
{
+ mutex_lock(&sbi->s_mmp_lock);
if (sbi->s_mmp_tsk) {
kthread_stop(sbi->s_mmp_tsk);
brelse(sbi->s_mmp_bh);
sbi->s_mmp_tsk = NULL;
}
+ mutex_unlock(&sbi->s_mmp_lock);
}
/*
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c3942804b57f..5bc3230553fb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4786,6 +4786,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
needs_recovery = (es->s_last_orphan != 0 ||
ext4_has_feature_journal_needs_recovery(sb));
+ mutex_init(&sbi->s_mmp_lock);
if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb))
if (ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)))
goto failed_mount3a;
--
2.31.1
After commit 618f003199c6 ("ext4: fix memory leak in
ext4_fill_super"), after the file system is remounted read-only, there
is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
point at freed memory, which the call to ext4_stop_mmpd() can trip
over.
Fix this by only allowing kmmpd() to exit when it is stopped via
ext4_stop_mmpd().
Link: https://lore.kernel.org/r/e525c0bf7b18da426bb3d3dd63830a3f85218a9e.1625244710.git.tytso@mit.edu
Reported-by: Ye Bin <[email protected]>
Bug-Report-Link: <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
Ye, thanks for reporting the bug! I think this might be a better way
to address the problem, since it avoids adding a mutex just to protect
s_mmp_tsk. What do you think?
fs/ext4/mmp.c | 31 +++++++++++++++++--------------
fs/ext4/super.c | 6 +++++-
2 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..af461df1c1ec 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -157,6 +157,16 @@ static int kmmpd(void *data)
sizeof(mmp->mmp_nodename));
while (!kthread_should_stop()) {
+ if (!(le32_to_cpu(es->s_feature_incompat) &
+ EXT4_FEATURE_INCOMPAT_MMP)) {
+ ext4_warning(sb, "kmmpd being stopped since MMP feature"
+ " has been disabled.");
+ goto wait_to_exit;
+ }
+ if (sb_rdonly(sb)) {
+ schedule_timeout_interruptible(HZ);
+ continue;
+ }
if (++seq > EXT4_MMP_SEQ_MAX)
seq = 1;
@@ -177,16 +187,6 @@ static int kmmpd(void *data)
failed_writes++;
}
- if (!(le32_to_cpu(es->s_feature_incompat) &
- EXT4_FEATURE_INCOMPAT_MMP)) {
- ext4_warning(sb, "kmmpd being stopped since MMP feature"
- " has been disabled.");
- goto exit_thread;
- }
-
- if (sb_rdonly(sb))
- break;
-
diff = jiffies - last_update_time;
if (diff < mmp_update_interval * HZ)
schedule_timeout_interruptible(mmp_update_interval *
@@ -207,7 +207,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, -retval,
"error reading MMP data: %d",
retval);
- goto exit_thread;
+ goto wait_to_exit;
}
mmp_check = (struct mmp_struct *)(bh_check->b_data);
@@ -221,7 +221,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, EBUSY, "abort");
put_bh(bh_check);
retval = -EBUSY;
- goto exit_thread;
+ goto wait_to_exit;
}
put_bh(bh_check);
}
@@ -246,6 +246,11 @@ static int kmmpd(void *data)
exit_thread:
return retval;
+wait_to_exit:
+ while (!kthread_should_stop())
+ schedule();
+ return retval;
+
}
void ext4_stop_mmpd(struct ext4_sb_info *sbi)
@@ -391,5 +396,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
brelse(bh);
return 1;
}
-
-
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
*/
ext4_mark_recovery_complete(sb, es);
}
- ext4_stop_mmpd(sbi);
} else {
/* Make sure we can mount this feature set readwrite */
if (ext4_has_feature_readonly(sb) ||
@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
ext4_release_system_zone(sb);
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
+
/*
* Some options can be enabled by ext4 and/or by VFS mount flag
* either way we need to make sure it matches in both *flags and
@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
for (i = 0; i < EXT4_MAXQUOTAS; i++)
kfree(to_free[i]);
#endif
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
kfree(orig_data);
return err;
}
--
2.31.0
Hi Theodore,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ext4/dev]
[also build test WARNING on next-20210701]
[cannot apply to v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: mips-randconfig-r021-20210702 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/0day-ci/linux/commit/37b4aa9eef5b3f07f803e18d4dba7aba46148f87
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
git checkout 37b4aa9eef5b3f07f803e18d4dba7aba46148f87
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/ext4/mmp.c:247:1: warning: unused label 'exit_thread' [-Wunused-label]
exit_thread:
^~~~~~~~~~~~
1 warning generated.
vim +/exit_thread +247 fs/ext4/mmp.c
c5e06d101aaf72 Johann Lombardi 2011-05-24 124
c5e06d101aaf72 Johann Lombardi 2011-05-24 125 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 126 * kmmpd will update the MMP sequence every s_mmp_update_interval seconds
c5e06d101aaf72 Johann Lombardi 2011-05-24 127 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 128 static int kmmpd(void *data)
c5e06d101aaf72 Johann Lombardi 2011-05-24 129 {
618f003199c618 Pavel Skripkin 2021-04-30 130 struct super_block *sb = (struct super_block *) data;
c5e06d101aaf72 Johann Lombardi 2011-05-24 131 struct ext4_super_block *es = EXT4_SB(sb)->s_es;
618f003199c618 Pavel Skripkin 2021-04-30 132 struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
c5e06d101aaf72 Johann Lombardi 2011-05-24 133 struct mmp_struct *mmp;
c5e06d101aaf72 Johann Lombardi 2011-05-24 134 ext4_fsblk_t mmp_block;
c5e06d101aaf72 Johann Lombardi 2011-05-24 135 u32 seq = 0;
c5e06d101aaf72 Johann Lombardi 2011-05-24 136 unsigned long failed_writes = 0;
c5e06d101aaf72 Johann Lombardi 2011-05-24 137 int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
c5e06d101aaf72 Johann Lombardi 2011-05-24 138 unsigned mmp_check_interval;
c5e06d101aaf72 Johann Lombardi 2011-05-24 139 unsigned long last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 140 unsigned long diff;
c5e06d101aaf72 Johann Lombardi 2011-05-24 141 int retval;
c5e06d101aaf72 Johann Lombardi 2011-05-24 142
c5e06d101aaf72 Johann Lombardi 2011-05-24 143 mmp_block = le64_to_cpu(es->s_mmp_block);
c5e06d101aaf72 Johann Lombardi 2011-05-24 144 mmp = (struct mmp_struct *)(bh->b_data);
af123b3718592a Arnd Bergmann 2018-07-29 145 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 146 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 147 * Start with the higher mmp_check_interval and reduce it if
c5e06d101aaf72 Johann Lombardi 2011-05-24 148 * the MMP block is being updated on time.
c5e06d101aaf72 Johann Lombardi 2011-05-24 149 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 150 mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
c5e06d101aaf72 Johann Lombardi 2011-05-24 151 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi 2011-05-24 152 mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
14c9ca0583eee8 Andreas Dilger 2020-01-26 153 BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
c5e06d101aaf72 Johann Lombardi 2011-05-24 154 bdevname(bh->b_bdev, mmp->mmp_bdevname);
c5e06d101aaf72 Johann Lombardi 2011-05-24 155
215fc6af739d2d Nikitas Angelinas 2011-10-18 156 memcpy(mmp->mmp_nodename, init_utsname()->nodename,
c5e06d101aaf72 Johann Lombardi 2011-05-24 157 sizeof(mmp->mmp_nodename));
c5e06d101aaf72 Johann Lombardi 2011-05-24 158
c5e06d101aaf72 Johann Lombardi 2011-05-24 159 while (!kthread_should_stop()) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 160 if (!(le32_to_cpu(es->s_feature_incompat) &
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 161 EXT4_FEATURE_INCOMPAT_MMP)) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 162 ext4_warning(sb, "kmmpd being stopped since MMP feature"
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 163 " has been disabled.");
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 164 goto wait_to_exit;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 165 }
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 166 if (sb_rdonly(sb)) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 167 schedule_timeout_interruptible(HZ);
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 168 continue;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 169 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 170 if (++seq > EXT4_MMP_SEQ_MAX)
c5e06d101aaf72 Johann Lombardi 2011-05-24 171 seq = 1;
c5e06d101aaf72 Johann Lombardi 2011-05-24 172
c5e06d101aaf72 Johann Lombardi 2011-05-24 173 mmp->mmp_seq = cpu_to_le32(seq);
af123b3718592a Arnd Bergmann 2018-07-29 174 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 175 last_update_time = jiffies;
c5e06d101aaf72 Johann Lombardi 2011-05-24 176
5c359a47e7d999 Darrick J. Wong 2012-04-29 177 retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi 2011-05-24 178 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 179 * Don't spew too many error messages. Print one every
c5e06d101aaf72 Johann Lombardi 2011-05-24 180 * (s_mmp_update_interval * 60) seconds.
c5e06d101aaf72 Johann Lombardi 2011-05-24 181 */
bdfc230f33a9da Nikitas Angelinas 2011-10-18 182 if (retval) {
878520ac45f9f6 Theodore Ts'o 2019-11-19 183 if ((failed_writes % 60) == 0) {
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 184 ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 185 "Error writing to MMP block");
878520ac45f9f6 Theodore Ts'o 2019-11-19 186 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 187 failed_writes++;
c5e06d101aaf72 Johann Lombardi 2011-05-24 188 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 189
c5e06d101aaf72 Johann Lombardi 2011-05-24 190 diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 191 if (diff < mmp_update_interval * HZ)
c5e06d101aaf72 Johann Lombardi 2011-05-24 192 schedule_timeout_interruptible(mmp_update_interval *
c5e06d101aaf72 Johann Lombardi 2011-05-24 193 HZ - diff);
c5e06d101aaf72 Johann Lombardi 2011-05-24 194
c5e06d101aaf72 Johann Lombardi 2011-05-24 195 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 196 * We need to make sure that more than mmp_check_interval
c5e06d101aaf72 Johann Lombardi 2011-05-24 197 * seconds have not passed since writing. If that has happened
c5e06d101aaf72 Johann Lombardi 2011-05-24 198 * we need to check if the MMP block is as we left it.
c5e06d101aaf72 Johann Lombardi 2011-05-24 199 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 200 diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 201 if (diff > mmp_check_interval * HZ) {
c5e06d101aaf72 Johann Lombardi 2011-05-24 202 struct buffer_head *bh_check = NULL;
c5e06d101aaf72 Johann Lombardi 2011-05-24 203 struct mmp_struct *mmp_check;
c5e06d101aaf72 Johann Lombardi 2011-05-24 204
c5e06d101aaf72 Johann Lombardi 2011-05-24 205 retval = read_mmp_block(sb, &bh_check, mmp_block);
c5e06d101aaf72 Johann Lombardi 2011-05-24 206 if (retval) {
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 207 ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 208 "error reading MMP data: %d",
c5e06d101aaf72 Johann Lombardi 2011-05-24 209 retval);
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 210 goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi 2011-05-24 211 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 212
c5e06d101aaf72 Johann Lombardi 2011-05-24 213 mmp_check = (struct mmp_struct *)(bh_check->b_data);
c5e06d101aaf72 Johann Lombardi 2011-05-24 214 if (mmp->mmp_seq != mmp_check->mmp_seq ||
c5e06d101aaf72 Johann Lombardi 2011-05-24 215 memcmp(mmp->mmp_nodename, mmp_check->mmp_nodename,
c5e06d101aaf72 Johann Lombardi 2011-05-24 216 sizeof(mmp->mmp_nodename))) {
c5e06d101aaf72 Johann Lombardi 2011-05-24 217 dump_mmp_msg(sb, mmp_check,
c5e06d101aaf72 Johann Lombardi 2011-05-24 218 "Error while updating MMP info. "
c5e06d101aaf72 Johann Lombardi 2011-05-24 219 "The filesystem seems to have been"
c5e06d101aaf72 Johann Lombardi 2011-05-24 220 " multiply mounted.");
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 221 ext4_error_err(sb, EBUSY, "abort");
0304688676bdfc vikram.jadhav07 2016-03-13 222 put_bh(bh_check);
0304688676bdfc vikram.jadhav07 2016-03-13 223 retval = -EBUSY;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 224 goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi 2011-05-24 225 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 226 put_bh(bh_check);
c5e06d101aaf72 Johann Lombardi 2011-05-24 227 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 228
c5e06d101aaf72 Johann Lombardi 2011-05-24 229 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 230 * Adjust the mmp_check_interval depending on how much time
c5e06d101aaf72 Johann Lombardi 2011-05-24 231 * it took for the MMP block to be written.
c5e06d101aaf72 Johann Lombardi 2011-05-24 232 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 233 mmp_check_interval = max(min(EXT4_MMP_CHECK_MULT * diff / HZ,
c5e06d101aaf72 Johann Lombardi 2011-05-24 234 EXT4_MMP_MAX_CHECK_INTERVAL),
c5e06d101aaf72 Johann Lombardi 2011-05-24 235 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi 2011-05-24 236 mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
c5e06d101aaf72 Johann Lombardi 2011-05-24 237 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 238
c5e06d101aaf72 Johann Lombardi 2011-05-24 239 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 240 * Unmount seems to be clean.
c5e06d101aaf72 Johann Lombardi 2011-05-24 241 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 242 mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
af123b3718592a Arnd Bergmann 2018-07-29 243 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 244
5c359a47e7d999 Darrick J. Wong 2012-04-29 245 retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi 2011-05-24 246
0304688676bdfc vikram.jadhav07 2016-03-13 @247 exit_thread:
c5e06d101aaf72 Johann Lombardi 2011-05-24 248 return retval;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 249 wait_to_exit:
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 250 while (!kthread_should_stop())
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 251 schedule();
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 252 return retval;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 253
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Theodore,
url: https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-m021-20210702 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
fs/ext4/mmp.c:252 kmmpd() error: uninitialized symbol 'retval'.
vim +/retval +252 fs/ext4/mmp.c
c5e06d101aaf72 Johann Lombardi 2011-05-24 128 static int kmmpd(void *data)
c5e06d101aaf72 Johann Lombardi 2011-05-24 129 {
618f003199c618 Pavel Skripkin 2021-04-30 130 struct super_block *sb = (struct super_block *) data;
c5e06d101aaf72 Johann Lombardi 2011-05-24 131 struct ext4_super_block *es = EXT4_SB(sb)->s_es;
618f003199c618 Pavel Skripkin 2021-04-30 132 struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
c5e06d101aaf72 Johann Lombardi 2011-05-24 133 struct mmp_struct *mmp;
c5e06d101aaf72 Johann Lombardi 2011-05-24 134 ext4_fsblk_t mmp_block;
c5e06d101aaf72 Johann Lombardi 2011-05-24 135 u32 seq = 0;
c5e06d101aaf72 Johann Lombardi 2011-05-24 136 unsigned long failed_writes = 0;
c5e06d101aaf72 Johann Lombardi 2011-05-24 137 int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
c5e06d101aaf72 Johann Lombardi 2011-05-24 138 unsigned mmp_check_interval;
c5e06d101aaf72 Johann Lombardi 2011-05-24 139 unsigned long last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 140 unsigned long diff;
c5e06d101aaf72 Johann Lombardi 2011-05-24 141 int retval;
c5e06d101aaf72 Johann Lombardi 2011-05-24 142
c5e06d101aaf72 Johann Lombardi 2011-05-24 143 mmp_block = le64_to_cpu(es->s_mmp_block);
c5e06d101aaf72 Johann Lombardi 2011-05-24 144 mmp = (struct mmp_struct *)(bh->b_data);
af123b3718592a Arnd Bergmann 2018-07-29 145 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 146 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 147 * Start with the higher mmp_check_interval and reduce it if
c5e06d101aaf72 Johann Lombardi 2011-05-24 148 * the MMP block is being updated on time.
c5e06d101aaf72 Johann Lombardi 2011-05-24 149 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 150 mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
c5e06d101aaf72 Johann Lombardi 2011-05-24 151 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi 2011-05-24 152 mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
14c9ca0583eee8 Andreas Dilger 2020-01-26 153 BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
c5e06d101aaf72 Johann Lombardi 2011-05-24 154 bdevname(bh->b_bdev, mmp->mmp_bdevname);
c5e06d101aaf72 Johann Lombardi 2011-05-24 155
215fc6af739d2d Nikitas Angelinas 2011-10-18 156 memcpy(mmp->mmp_nodename, init_utsname()->nodename,
c5e06d101aaf72 Johann Lombardi 2011-05-24 157 sizeof(mmp->mmp_nodename));
c5e06d101aaf72 Johann Lombardi 2011-05-24 158
c5e06d101aaf72 Johann Lombardi 2011-05-24 159 while (!kthread_should_stop()) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 160 if (!(le32_to_cpu(es->s_feature_incompat) &
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 161 EXT4_FEATURE_INCOMPAT_MMP)) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 162 ext4_warning(sb, "kmmpd being stopped since MMP feature"
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 163 " has been disabled.");
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 164 goto wait_to_exit;
Smatch complains about this goto.
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 165 }
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 166 if (sb_rdonly(sb)) {
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 167 schedule_timeout_interruptible(HZ);
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 168 continue;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 169 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 170 if (++seq > EXT4_MMP_SEQ_MAX)
c5e06d101aaf72 Johann Lombardi 2011-05-24 171 seq = 1;
c5e06d101aaf72 Johann Lombardi 2011-05-24 172
c5e06d101aaf72 Johann Lombardi 2011-05-24 173 mmp->mmp_seq = cpu_to_le32(seq);
af123b3718592a Arnd Bergmann 2018-07-29 174 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 175 last_update_time = jiffies;
c5e06d101aaf72 Johann Lombardi 2011-05-24 176
5c359a47e7d999 Darrick J. Wong 2012-04-29 177 retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi 2011-05-24 178 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 179 * Don't spew too many error messages. Print one every
c5e06d101aaf72 Johann Lombardi 2011-05-24 180 * (s_mmp_update_interval * 60) seconds.
c5e06d101aaf72 Johann Lombardi 2011-05-24 181 */
bdfc230f33a9da Nikitas Angelinas 2011-10-18 182 if (retval) {
878520ac45f9f6 Theodore Ts'o 2019-11-19 183 if ((failed_writes % 60) == 0) {
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 184 ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 185 "Error writing to MMP block");
878520ac45f9f6 Theodore Ts'o 2019-11-19 186 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 187 failed_writes++;
c5e06d101aaf72 Johann Lombardi 2011-05-24 188 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 189
c5e06d101aaf72 Johann Lombardi 2011-05-24 190 diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 191 if (diff < mmp_update_interval * HZ)
c5e06d101aaf72 Johann Lombardi 2011-05-24 192 schedule_timeout_interruptible(mmp_update_interval *
c5e06d101aaf72 Johann Lombardi 2011-05-24 193 HZ - diff);
c5e06d101aaf72 Johann Lombardi 2011-05-24 194
c5e06d101aaf72 Johann Lombardi 2011-05-24 195 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 196 * We need to make sure that more than mmp_check_interval
c5e06d101aaf72 Johann Lombardi 2011-05-24 197 * seconds have not passed since writing. If that has happened
c5e06d101aaf72 Johann Lombardi 2011-05-24 198 * we need to check if the MMP block is as we left it.
c5e06d101aaf72 Johann Lombardi 2011-05-24 199 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 200 diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi 2011-05-24 201 if (diff > mmp_check_interval * HZ) {
c5e06d101aaf72 Johann Lombardi 2011-05-24 202 struct buffer_head *bh_check = NULL;
c5e06d101aaf72 Johann Lombardi 2011-05-24 203 struct mmp_struct *mmp_check;
c5e06d101aaf72 Johann Lombardi 2011-05-24 204
c5e06d101aaf72 Johann Lombardi 2011-05-24 205 retval = read_mmp_block(sb, &bh_check, mmp_block);
c5e06d101aaf72 Johann Lombardi 2011-05-24 206 if (retval) {
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 207 ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 208 "error reading MMP data: %d",
c5e06d101aaf72 Johann Lombardi 2011-05-24 209 retval);
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 210 goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi 2011-05-24 211 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 212
c5e06d101aaf72 Johann Lombardi 2011-05-24 213 mmp_check = (struct mmp_struct *)(bh_check->b_data);
c5e06d101aaf72 Johann Lombardi 2011-05-24 214 if (mmp->mmp_seq != mmp_check->mmp_seq ||
c5e06d101aaf72 Johann Lombardi 2011-05-24 215 memcmp(mmp->mmp_nodename, mmp_check->mmp_nodename,
c5e06d101aaf72 Johann Lombardi 2011-05-24 216 sizeof(mmp->mmp_nodename))) {
c5e06d101aaf72 Johann Lombardi 2011-05-24 217 dump_mmp_msg(sb, mmp_check,
c5e06d101aaf72 Johann Lombardi 2011-05-24 218 "Error while updating MMP info. "
c5e06d101aaf72 Johann Lombardi 2011-05-24 219 "The filesystem seems to have been"
c5e06d101aaf72 Johann Lombardi 2011-05-24 220 " multiply mounted.");
54d3adbc29f0c7 Theodore Ts'o 2020-03-28 221 ext4_error_err(sb, EBUSY, "abort");
0304688676bdfc vikram.jadhav07 2016-03-13 222 put_bh(bh_check);
0304688676bdfc vikram.jadhav07 2016-03-13 223 retval = -EBUSY;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 224 goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi 2011-05-24 225 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 226 put_bh(bh_check);
c5e06d101aaf72 Johann Lombardi 2011-05-24 227 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 228
c5e06d101aaf72 Johann Lombardi 2011-05-24 229 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 230 * Adjust the mmp_check_interval depending on how much time
c5e06d101aaf72 Johann Lombardi 2011-05-24 231 * it took for the MMP block to be written.
c5e06d101aaf72 Johann Lombardi 2011-05-24 232 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 233 mmp_check_interval = max(min(EXT4_MMP_CHECK_MULT * diff / HZ,
c5e06d101aaf72 Johann Lombardi 2011-05-24 234 EXT4_MMP_MAX_CHECK_INTERVAL),
c5e06d101aaf72 Johann Lombardi 2011-05-24 235 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi 2011-05-24 236 mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
c5e06d101aaf72 Johann Lombardi 2011-05-24 237 }
c5e06d101aaf72 Johann Lombardi 2011-05-24 238
c5e06d101aaf72 Johann Lombardi 2011-05-24 239 /*
c5e06d101aaf72 Johann Lombardi 2011-05-24 240 * Unmount seems to be clean.
c5e06d101aaf72 Johann Lombardi 2011-05-24 241 */
c5e06d101aaf72 Johann Lombardi 2011-05-24 242 mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
af123b3718592a Arnd Bergmann 2018-07-29 243 mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi 2011-05-24 244
5c359a47e7d999 Darrick J. Wong 2012-04-29 245 retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi 2011-05-24 246
0304688676bdfc vikram.jadhav07 2016-03-13 247 exit_thread:
c5e06d101aaf72 Johann Lombardi 2011-05-24 248 return retval;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 249 wait_to_exit:
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 250 while (!kthread_should_stop())
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 251 schedule();
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 @252 return retval;
37b4aa9eef5b3f Theodore Ts'o 2021-07-02 253
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue 29-06-21 22:36:03, Ye Bin wrote:
> Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
> already running and even exit as exception. Even though we set
> sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
> "sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
> mount kmmpd
> | |
> |-call kthread_run |
> | |-kmmpd runing
> | |-kmmpd exit sbi->s_mmp_tsk=NULL
> | |
> |-kthread_run return |
> | and set sbi->s_mmp_tsk |
> | |
> |-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF
>
> This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
> Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
> NULL. We can create kthread first, and then wakeup kmmpd kthread later.
>
> Signed-off-by: Ye Bin <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/ext4/mmp.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index fc18a8c205c7..6ec1ea182cc0 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -394,16 +394,18 @@ int ext4_multi_mount_protect(struct super_block *sb,
> /*
> * Start a kernel thread to update the MMP block periodically.
> */
> - EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, sb, "kmmpd-%.*s",
> - (int)sizeof(mmp->mmp_bdevname),
> - bdevname(bh->b_bdev,
> - mmp->mmp_bdevname));
> + EXT4_SB(sb)->s_mmp_tsk = kthread_create(kmmpd, sb, "kmmpd-%.*s",
> + (int)sizeof(mmp->mmp_bdevname),
> + bdevname(bh->b_bdev,
> + mmp->mmp_bdevname));
> +
> if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
> EXT4_SB(sb)->s_mmp_tsk = NULL;
> ext4_warning(sb, "Unable to create kmmpd thread for %s.",
> sb->s_id);
> goto failed;
> }
> + wake_up_process(EXT4_SB(sb)->s_mmp_tsk);
>
> return 0;
>
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue 29-06-21 22:36:02, Ye Bin wrote:
> After merge 618f003199c6("ext4: fix memory leak in ext4_fill_super") commit,
> we add delay in ext4_remount after "sb->s_flags |= SB_RDONLY", then
> remount filesystem with read-only kasan report following warning:
> [ 888.695326] ==================================================================
> [ 888.696566] BUG: KASAN: use-after-free in kthread_stop+0x4c/0x2f0
> [ 888.697599] Write of size 4 at addr ffff8883849e0020 by task mount/2013
> [ 888.698707]
> [ 888.698982] CPU: 4 PID: 2013 Comm: mount Not tainted 4.19.95-00013-ga369a6189bbf-dirty #413
> [ 888.700376] Hardware name: QEMU Standard PC
> [ 888.702587] Call Trace:
> [ 888.703017] dump_stack+0x108/0x15f
> [ 888.703615] print_address_description+0xa5/0x372
> [ 888.704420] kasan_report.cold+0x236/0x2a8
> [ 888.705761] check_memory_region+0x240/0x270
> [ 888.706486] kasan_check_write+0x20/0x30
> [ 888.707156] kthread_stop+0x4c/0x2f0
> [ 888.707776] ext4_stop_mmpd+0x32/0x90
> [ 888.708262] ext4_remount.cold+0xf6/0x116
> [ 888.712671] do_remount_sb+0xff/0x460
> [ 888.714007] do_mount+0xce3/0x1be0
> [ 888.717749] ksys_mount+0xb2/0x150
> [ 888.718163] __x64_sys_mount+0x6a/0x80
> [ 888.718607] do_syscall_64+0xd9/0x1f0
> [ 888.719047] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> As kmmpd will exit if filesystem is read-only. Then sbi->s_mmp_tsk become wild
> ptr, lead to use-after-free. As kmmpd will exit by others(call ktread_stop)
> or by itself. After 618f003199c6 commit we can trigger this issue very easy.
> Before this commit also exist this issue.
> If kmmpd exit by itself, after merge 618f003199c6 commit there will trigger UAF
> when umount filesystem.
> To fix this issue, introduce sbi->s_mmp_lock to protect sbi->s_mmp_tsk. If kmmpd
> exit by itself, we set sbi->s_mmp_tsk with NULL, and release mmp buffer_head.
>
> Fixes: 618f003199c6 ("ext4: fix memory leak in ext4_fill_super")
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/ext4/ext4.h | 1 +
> fs/ext4/mmp.c | 24 ++++++++++++++++++++++--
> fs/ext4/super.c | 1 +
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 8d3446746718..a479da37fed4 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1489,6 +1489,7 @@ struct ext4_sb_info {
> struct completion s_kobj_unregister;
> struct super_block *s_sb;
> struct buffer_head *s_mmp_bh;
> + struct mutex s_mmp_lock;
>
> /* Journaling */
> struct journal_s *s_journal;
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 6cb598b549ca..fc18a8c205c7 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -128,8 +128,9 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
> static int kmmpd(void *data)
> {
> struct super_block *sb = (struct super_block *) data;
> - struct ext4_super_block *es = EXT4_SB(sb)->s_es;
> - struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
> + struct ext4_sb_info *sbi = EXT4_SB(sb);
> + struct ext4_super_block *es = sbi->s_es;
> + struct buffer_head *bh = sbi->s_mmp_bh;
> struct mmp_struct *mmp;
> ext4_fsblk_t mmp_block;
> u32 seq = 0;
> @@ -245,16 +246,35 @@ static int kmmpd(void *data)
> retval = write_mmp_block(sb, bh);
>
> exit_thread:
> + /*
> + * Maybe s_mmp_tsk kthread is stoped by others or by itself. If exit
> + * by itself then sbi->s_mmp_tsk will be wild ptr, so there is need
> + * set sbi->s_mmp_tsk with NULL, and also release mmp buffer_head.
> + */
> + while (!kthread_should_stop()) {
> + if (!mutex_trylock(&sbi->s_mmp_lock))
> + continue;
> +
> + if (sbi->s_mmp_tsk) {
> + sbi->s_mmp_tsk = NULL;
> + brelse(bh);
> + }
> + mutex_unlock(&sbi->s_mmp_lock);
> + break;
> + }
> +
> return retval;
> }
Honestly, this is just ugly. I think it would be saner if ext4_stop_mmpd()
did:
void ext4_stop_mmpd(struct ext4_sb_info *sbi)
{
struct task_struct *tsk;
mutex_lock(&sbi->s_mmp_lock);
tsk = sbi->s_mmp_tsk;
if (tsk) {
sbi->s_mmp_tsk = NULL;
get_task_struct(tsk);
}
mutex_unlock(&sbi->s_mmp_lock);
if (tsk) {
kthread_stop(tsk);
put_task_struct(k);
}
}
And you leave freeing of 'bh' to exit path from kmmpd() which can also use
normal locking scheme for zeroing s_mmp_tsk now.
That being said for this scheme spinlock is enough, you don't need a mutex
for s_mmp_lock.
Honza
> void ext4_stop_mmpd(struct ext4_sb_info *sbi)
> {
> + mutex_lock(&sbi->s_mmp_lock);
> if (sbi->s_mmp_tsk) {
> kthread_stop(sbi->s_mmp_tsk);
> brelse(sbi->s_mmp_bh);
> sbi->s_mmp_tsk = NULL;
> }
> + mutex_unlock(&sbi->s_mmp_lock);
> }
>
> /*
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c3942804b57f..5bc3230553fb 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4786,6 +4786,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> needs_recovery = (es->s_last_orphan != 0 ||
> ext4_has_feature_journal_needs_recovery(sb));
>
> + mutex_init(&sbi->s_mmp_lock);
> if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb))
> if (ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)))
> goto failed_mount3a;
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Jun 29, 2021, at 8:36 AM, Ye Bin <[email protected]> wrote:
>
> Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
> already running and even exit as exception. Even though we set
> sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
> "sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
> mount kmmpd
> | |
> |-call kthread_run |
> | |-kmmpd runing
> | |-kmmpd exit sbi->s_mmp_tsk=NULL
> | |
> |-kthread_run return |
> | and set sbi->s_mmp_tsk |
> | |
> |-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF
>
> This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
> Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
> NULL. We can create kthread first, and then wakeup kmmpd kthread later.
>
> Signed-off-by: Ye Bin <[email protected]>
Please note minor typo in the patch subject, should be "use-after-free".
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> fs/ext4/mmp.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index fc18a8c205c7..6ec1ea182cc0 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -394,16 +394,18 @@ int ext4_multi_mount_protect(struct super_block *sb,
> /*
> * Start a kernel thread to update the MMP block periodically.
> */
> - EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, sb, "kmmpd-%.*s",
> - (int)sizeof(mmp->mmp_bdevname),
> - bdevname(bh->b_bdev,
> - mmp->mmp_bdevname));
> + EXT4_SB(sb)->s_mmp_tsk = kthread_create(kmmpd, sb, "kmmpd-%.*s",
> + (int)sizeof(mmp->mmp_bdevname),
> + bdevname(bh->b_bdev,
> + mmp->mmp_bdevname));
> +
> if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
> EXT4_SB(sb)->s_mmp_tsk = NULL;
> ext4_warning(sb, "Unable to create kmmpd thread for %s.",
> sb->s_id);
> goto failed;
> }
> + wake_up_process(EXT4_SB(sb)->s_mmp_tsk);
>
> return 0;
>
> --
> 2.31.1
>
Cheers, Andreas
On Mon 05-07-21 16:35:28, Theodore Ts'o wrote:
> On Mon, Jul 05, 2021 at 01:15:48PM +0200, Jan Kara wrote:
> >
> > That being said for this scheme spinlock is enough, you don't need a mutex
> > for s_mmp_lock.
>
> I think we can solve this without using using either a spinlock or a
> mutex, and it's a smaller and simpler patch as a result. (This is the
> -v2 version of this patch, which removes an unused label compared to
> the earlier version.)
Yeah, what you suggest is probably simpler. Some comments below.
> From 22ebc97aac75e27a5fd11acdb2bc3030d1da58d1 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <[email protected]>
> Date: Fri, 2 Jul 2021 12:45:02 -0400
> Subject: [PATCH] ext4: fix possible UAF when remounting r/o a mmp-protected file system
>
> After commit 618f003199c6 ("ext4: fix memory leak in
> ext4_fill_super"), after the file system is remounted read-only, there
> is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
> point at freed memory, which the call to ext4_stop_mmpd() can trip
> over.
>
> Fix this by only allowing kmmpd() to exit when it is stopped via
> ext4_stop_mmpd().
>
> Link: https://lore.kernel.org/r/e525c0bf7b18da426bb3d3dd63830a3f85218a9e.1625244710.git.tytso@mit.edu
> Reported-by: Ye Bin <[email protected]>
> Bug-Report-Link: <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> fs/ext4/mmp.c | 33 +++++++++++++++++----------------
> fs/ext4/super.c | 6 +++++-
> 2 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 6cb598b549ca..1e95cee3d8b7 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -157,6 +157,17 @@ static int kmmpd(void *data)
> sizeof(mmp->mmp_nodename));
>
> while (!kthread_should_stop()) {
> + if (!(le32_to_cpu(es->s_feature_incompat) &
> + EXT4_FEATURE_INCOMPAT_MMP)) {
We can probably use ext4_has_feature_mmp() macro when changing this?
> + ext4_warning(sb, "kmmpd being stopped since MMP feature"
> + " has been disabled.");
> + goto wait_to_exit;
> + }
> + if (sb_rdonly(sb)) {
> + if (!kthread_should_stop())
> + schedule_timeout_interruptible(HZ);
Cannot this effectively block remount RO for 1s when we wait for kmmpd to
exit? I think doing 'break' when we detected RO super is fine. We'll write
the mmp block and then wait for kthread_should_stop() condition as in any
other abort case. Am I missing something?
> + continue;
> + }
> if (++seq > EXT4_MMP_SEQ_MAX)
> seq = 1;
>
> @@ -177,16 +188,6 @@ static int kmmpd(void *data)
> failed_writes++;
> }
>
> - if (!(le32_to_cpu(es->s_feature_incompat) &
> - EXT4_FEATURE_INCOMPAT_MMP)) {
> - ext4_warning(sb, "kmmpd being stopped since MMP feature"
> - " has been disabled.");
> - goto exit_thread;
> - }
> -
> - if (sb_rdonly(sb))
> - break;
> -
> diff = jiffies - last_update_time;
> if (diff < mmp_update_interval * HZ)
> schedule_timeout_interruptible(mmp_update_interval *
> @@ -207,7 +208,7 @@ static int kmmpd(void *data)
> ext4_error_err(sb, -retval,
> "error reading MMP data: %d",
> retval);
> - goto exit_thread;
> + goto wait_to_exit;
> }
>
> mmp_check = (struct mmp_struct *)(bh_check->b_data);
> @@ -221,7 +222,7 @@ static int kmmpd(void *data)
> ext4_error_err(sb, EBUSY, "abort");
> put_bh(bh_check);
> retval = -EBUSY;
> - goto exit_thread;
> + goto wait_to_exit;
> }
> put_bh(bh_check);
> }
> @@ -242,9 +243,11 @@ static int kmmpd(void *data)
> mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
> mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
>
> - retval = write_mmp_block(sb, bh);
> + return write_mmp_block(sb, bh);
>
> -exit_thread:
> +wait_to_exit:
> + while (!kthread_should_stop())
> + schedule();
This makes me a bit nervous that we could unnecessarily burn CPU for
potentially a long time (e.g. if somebody uses tune2fs to disable MMP, we
would be sitting in this loop until the fs in remounted / unmounted). So
maybe we should have something like:
while (!kthread_should_stop()) {
set_task_state(TASK_INTERRUPTIBLE);
if (!kthread_should_stop())
schedule();
}
This should safely synchronize with (and not miss wakeup from)
kthread_stop() since that first sets KTHREAD_SHOULD_STOP and after that
calls wake_up_process().
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, Jul 06, 2021 at 01:11:37PM +0200, Jan Kara wrote:
> > --- a/fs/ext4/mmp.c
> > +++ b/fs/ext4/mmp.c
> > @@ -157,6 +157,17 @@ static int kmmpd(void *data)
> > sizeof(mmp->mmp_nodename));
> >
> > while (!kthread_should_stop()) {
> > + if (!(le32_to_cpu(es->s_feature_incompat) &
> > + EXT4_FEATURE_INCOMPAT_MMP)) {
>
> We can probably use ext4_has_feature_mmp() macro when changing this?
Ack, I'll make that change.
> > + if (sb_rdonly(sb)) {
> > + if (!kthread_should_stop())
> > + schedule_timeout_interruptible(HZ);
>
> Cannot this effectively block remount RO for 1s when we wait for kmmpd to
> exit? I think doing 'break' when we detected RO super is fine. We'll write
> the mmp block and then wait for kthread_should_stop() condition as in any
> other abort case. Am I missing something?
Yeah, we do want to update the mmp block when remounting the file
system read-only. So breaking out to exit is the right thing to do
here.
> > +wait_to_exit:
> > + while (!kthread_should_stop())
> > + schedule();
>
> This makes me a bit nervous that we could unnecessarily burn CPU for
> potentially a long time (e.g. if somebody uses tune2fs to disable MMP, we
> would be sitting in this loop until the fs in remounted / unmounted). So
> maybe we should have something like:
>
> while (!kthread_should_stop()) {
> set_task_state(TASK_INTERRUPTIBLE);
> if (!kthread_should_stop())
> schedule();
> }
>
> This should safely synchronize with (and not miss wakeup from)
> kthread_stop() since that first sets KTHREAD_SHOULD_STOP and after that
> calls wake_up_process().
Yep, good catch. I'll fix this and send out revised patch.
- Ted
After commit 618f003199c6 ("ext4: fix memory leak in
ext4_fill_super"), after the file system is remounted read-only, there
is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
point at freed memory, which the call to ext4_stop_mmpd() can trip
over.
Fix this by only allowing kmmpd() to exit when it is stopped via
ext4_stop_mmpd().
Link: https://lore.kernel.org/r/[email protected]
Reported-by: Ye Bin <[email protected]>
Bug-Report-Link: <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/mmp.c | 31 ++++++++++++++-----------------
fs/ext4/super.c | 6 +++++-
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..16d69c0ff46a 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -156,7 +156,12 @@ static int kmmpd(void *data)
memcpy(mmp->mmp_nodename, init_utsname()->nodename,
sizeof(mmp->mmp_nodename));
- while (!kthread_should_stop()) {
+ while (!kthread_should_stop() && !sb_rdonly(sb)) {
+ if (!ext4_has_feature_mmp(sb)) {
+ ext4_warning(sb, "kmmpd being stopped since MMP feature"
+ " has been disabled.");
+ goto wait_to_exit;
+ }
if (++seq > EXT4_MMP_SEQ_MAX)
seq = 1;
@@ -177,16 +182,6 @@ static int kmmpd(void *data)
failed_writes++;
}
- if (!(le32_to_cpu(es->s_feature_incompat) &
- EXT4_FEATURE_INCOMPAT_MMP)) {
- ext4_warning(sb, "kmmpd being stopped since MMP feature"
- " has been disabled.");
- goto exit_thread;
- }
-
- if (sb_rdonly(sb))
- break;
-
diff = jiffies - last_update_time;
if (diff < mmp_update_interval * HZ)
schedule_timeout_interruptible(mmp_update_interval *
@@ -207,7 +202,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, -retval,
"error reading MMP data: %d",
retval);
- goto exit_thread;
+ goto wait_to_exit;
}
mmp_check = (struct mmp_struct *)(bh_check->b_data);
@@ -221,7 +216,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, EBUSY, "abort");
put_bh(bh_check);
retval = -EBUSY;
- goto exit_thread;
+ goto wait_to_exit;
}
put_bh(bh_check);
}
@@ -242,9 +237,13 @@ static int kmmpd(void *data)
mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
- retval = write_mmp_block(sb, bh);
+ return write_mmp_block(sb, bh);
-exit_thread:
+wait_to_exit:
+ set_current_state(TASK_INTERRUPTIBLE);
+ while (!kthread_should_stop())
+ schedule();
+ set_current_state(TASK_RUNNING);
return retval;
}
@@ -391,5 +390,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
brelse(bh);
return 1;
}
-
-
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
*/
ext4_mark_recovery_complete(sb, es);
}
- ext4_stop_mmpd(sbi);
} else {
/* Make sure we can mount this feature set readwrite */
if (ext4_has_feature_readonly(sb) ||
@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
ext4_release_system_zone(sb);
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
+
/*
* Some options can be enabled by ext4 and/or by VFS mount flag
* either way we need to make sure it matches in both *flags and
@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
for (i = 0; i < EXT4_MAXQUOTAS; i++)
kfree(to_free[i]);
#endif
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
kfree(orig_data);
return err;
}
--
2.31.0
On Tue 06-07-21 13:12:08, Theodore Ts'o wrote:
> After commit 618f003199c6 ("ext4: fix memory leak in
> ext4_fill_super"), after the file system is remounted read-only, there
> is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
> point at freed memory, which the call to ext4_stop_mmpd() can trip
> over.
>
> Fix this by only allowing kmmpd() to exit when it is stopped via
> ext4_stop_mmpd().
>
> Link: https://lore.kernel.org/r/[email protected]
> Reported-by: Ye Bin <[email protected]>
> Bug-Report-Link: <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
The patch looks mostly fine. Two comments below.
> @@ -242,9 +237,13 @@ static int kmmpd(void *data)
> mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
> mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
>
> - retval = write_mmp_block(sb, bh);
> + return write_mmp_block(sb, bh);
I think we need to keep retval = write_mmp_block() here. Otherwise we could
exit early in sb_rdonly() case and still have potential use-after-free.
>
> -exit_thread:
> +wait_to_exit:
> + set_current_state(TASK_INTERRUPTIBLE);
> + while (!kthread_should_stop())
> + schedule();
> + set_current_state(TASK_RUNNING);
> return retval;
> }
This is more or less fine but if we get a spurious wakeup for whatever
reason (which sets task to TASK_RUNNING state) we would still be
potentially looping in that loop burning cpu...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
After commit 618f003199c6 ("ext4: fix memory leak in
ext4_fill_super"), after the file system is remounted read-only, there
is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
point at freed memory, which the call to ext4_stop_mmpd() can trip
over.
Fix this by only allowing kmmpd() to exit when it is stopped via
ext4_stop_mmpd().
Link: https://lore.kernel.org/r/[email protected]
Reported-by: Ye Bin <[email protected]>
Bug-Report-Link: <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/mmp.c | 31 +++++++++++++++----------------
fs/ext4/super.c | 6 +++++-
2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..bc364c119af6 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -156,7 +156,12 @@ static int kmmpd(void *data)
memcpy(mmp->mmp_nodename, init_utsname()->nodename,
sizeof(mmp->mmp_nodename));
- while (!kthread_should_stop()) {
+ while (!kthread_should_stop() && !sb_rdonly(sb)) {
+ if (!ext4_has_feature_mmp(sb)) {
+ ext4_warning(sb, "kmmpd being stopped since MMP feature"
+ " has been disabled.");
+ goto wait_to_exit;
+ }
if (++seq > EXT4_MMP_SEQ_MAX)
seq = 1;
@@ -177,16 +182,6 @@ static int kmmpd(void *data)
failed_writes++;
}
- if (!(le32_to_cpu(es->s_feature_incompat) &
- EXT4_FEATURE_INCOMPAT_MMP)) {
- ext4_warning(sb, "kmmpd being stopped since MMP feature"
- " has been disabled.");
- goto exit_thread;
- }
-
- if (sb_rdonly(sb))
- break;
-
diff = jiffies - last_update_time;
if (diff < mmp_update_interval * HZ)
schedule_timeout_interruptible(mmp_update_interval *
@@ -207,7 +202,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, -retval,
"error reading MMP data: %d",
retval);
- goto exit_thread;
+ goto wait_to_exit;
}
mmp_check = (struct mmp_struct *)(bh_check->b_data);
@@ -221,7 +216,7 @@ static int kmmpd(void *data)
ext4_error_err(sb, EBUSY, "abort");
put_bh(bh_check);
retval = -EBUSY;
- goto exit_thread;
+ goto wait_to_exit;
}
put_bh(bh_check);
}
@@ -244,7 +239,13 @@ static int kmmpd(void *data)
retval = write_mmp_block(sb, bh);
-exit_thread:
+wait_to_exit:
+ while (!kthread_should_stop()) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (!kthread_should_stop())
+ schedule();
+ }
+ set_current_state(TASK_RUNNING);
return retval;
}
@@ -391,5 +392,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
brelse(bh);
return 1;
}
-
-
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
*/
ext4_mark_recovery_complete(sb, es);
}
- ext4_stop_mmpd(sbi);
} else {
/* Make sure we can mount this feature set readwrite */
if (ext4_has_feature_readonly(sb) ||
@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
ext4_release_system_zone(sb);
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
+
/*
* Some options can be enabled by ext4 and/or by VFS mount flag
* either way we need to make sure it matches in both *flags and
@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
for (i = 0; i < EXT4_MAXQUOTAS; i++)
kfree(to_free[i]);
#endif
+ if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+ ext4_stop_mmpd(sbi);
kfree(orig_data);
return err;
}
--
2.31.0
On Tue 06-07-21 20:24:33, Theodore Ts'o wrote:
> After commit 618f003199c6 ("ext4: fix memory leak in
> ext4_fill_super"), after the file system is remounted read-only, there
> is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
> point at freed memory, which the call to ext4_stop_mmpd() can trip
> over.
>
> Fix this by only allowing kmmpd() to exit when it is stopped via
> ext4_stop_mmpd().
>
> Link: https://lore.kernel.org/r/[email protected]
> Reported-by: Ye Bin <[email protected]>
> Bug-Report-Link: <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/ext4/mmp.c | 31 +++++++++++++++----------------
> fs/ext4/super.c | 6 +++++-
> 2 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 6cb598b549ca..bc364c119af6 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -156,7 +156,12 @@ static int kmmpd(void *data)
> memcpy(mmp->mmp_nodename, init_utsname()->nodename,
> sizeof(mmp->mmp_nodename));
>
> - while (!kthread_should_stop()) {
> + while (!kthread_should_stop() && !sb_rdonly(sb)) {
> + if (!ext4_has_feature_mmp(sb)) {
> + ext4_warning(sb, "kmmpd being stopped since MMP feature"
> + " has been disabled.");
> + goto wait_to_exit;
> + }
> if (++seq > EXT4_MMP_SEQ_MAX)
> seq = 1;
>
> @@ -177,16 +182,6 @@ static int kmmpd(void *data)
> failed_writes++;
> }
>
> - if (!(le32_to_cpu(es->s_feature_incompat) &
> - EXT4_FEATURE_INCOMPAT_MMP)) {
> - ext4_warning(sb, "kmmpd being stopped since MMP feature"
> - " has been disabled.");
> - goto exit_thread;
> - }
> -
> - if (sb_rdonly(sb))
> - break;
> -
> diff = jiffies - last_update_time;
> if (diff < mmp_update_interval * HZ)
> schedule_timeout_interruptible(mmp_update_interval *
> @@ -207,7 +202,7 @@ static int kmmpd(void *data)
> ext4_error_err(sb, -retval,
> "error reading MMP data: %d",
> retval);
> - goto exit_thread;
> + goto wait_to_exit;
> }
>
> mmp_check = (struct mmp_struct *)(bh_check->b_data);
> @@ -221,7 +216,7 @@ static int kmmpd(void *data)
> ext4_error_err(sb, EBUSY, "abort");
> put_bh(bh_check);
> retval = -EBUSY;
> - goto exit_thread;
> + goto wait_to_exit;
> }
> put_bh(bh_check);
> }
> @@ -244,7 +239,13 @@ static int kmmpd(void *data)
>
> retval = write_mmp_block(sb, bh);
>
> -exit_thread:
> +wait_to_exit:
> + while (!kthread_should_stop()) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (!kthread_should_stop())
> + schedule();
> + }
> + set_current_state(TASK_RUNNING);
> return retval;
> }
>
> @@ -391,5 +392,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
> brelse(bh);
> return 1;
> }
> -
> -
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cdbe71d935e8..b8ff0399e171 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> */
> ext4_mark_recovery_complete(sb, es);
> }
> - ext4_stop_mmpd(sbi);
> } else {
> /* Make sure we can mount this feature set readwrite */
> if (ext4_has_feature_readonly(sb) ||
> @@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
> ext4_release_system_zone(sb);
>
> + if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
> + ext4_stop_mmpd(sbi);
> +
> /*
> * Some options can be enabled by ext4 and/or by VFS mount flag
> * either way we need to make sure it matches in both *flags and
> @@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> kfree(to_free[i]);
> #endif
> + if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
> + ext4_stop_mmpd(sbi);
> kfree(orig_data);
> return err;
> }
> --
> 2.31.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR