jfs: hold LOG_LOCK on umount to avoid null-ptr-deref

[PATCH] jfs: hold LOG_LOCK on umount to avoid null-ptr-deref

Posted by Helen Koike 1 month, 2 weeks ago

write_special_inodes() function iterate through the log->sb_list and
access the sbi fields, which can be set to NULL concurrently by umount.

Fix concurrency issue by holding LOG_LOCK and checking for NULL.

Reported-by: syzbot+e14b1036481911ae4d77@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e14b1036481911ae4d77
Signed-off-by: Helen Koike <koike@igalia.com>
---
 fs/jfs/jfs_logmgr.c | 16 +++++++---------
 fs/jfs/jfs_logmgr.h |  7 +++++++
 fs/jfs/jfs_umount.c | 10 ++++++++++
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index 5b1c5da04163..59f94c28007d 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -74,12 +74,6 @@ static struct lbuf *log_redrive_list;
 static DEFINE_SPINLOCK(log_redrive_lock);
 
 
-/*
- *	log read/write serialization (per log)
- */
-#define LOG_LOCK_INIT(log)	mutex_init(&(log)->loglock)
-#define LOG_LOCK(log)		mutex_lock(&((log)->loglock))
-#define LOG_UNLOCK(log)		mutex_unlock(&((log)->loglock))
 
 
 /*
@@ -204,9 +198,13 @@ static void write_special_inodes(struct jfs_log *log,
 	struct jfs_sb_info *sbi;
 
 	list_for_each_entry(sbi, &log->sb_list, log_list) {
-		writer(sbi->ipbmap->i_mapping);
-		writer(sbi->ipimap->i_mapping);
-		writer(sbi->direct_inode->i_mapping);
+		/* These pointers can be NULL before list_del during umount */
+		if (sbi->ipbmap)
+			writer(sbi->ipbmap->i_mapping);
+		if (sbi->ipimap)
+			writer(sbi->ipimap->i_mapping);
+		if (sbi->direct_inode)
+			writer(sbi->direct_inode->i_mapping);
 	}
 }
 
diff --git a/fs/jfs/jfs_logmgr.h b/fs/jfs/jfs_logmgr.h
index 8b8994e48cd0..09e0ef6aecce 100644
--- a/fs/jfs/jfs_logmgr.h
+++ b/fs/jfs/jfs_logmgr.h
@@ -402,6 +402,13 @@ struct jfs_log {
 	int no_integrity;	/* 3: flag to disable journaling to disk */
 };
 
+/*
+ * log read/write serialization (per log)
+ */
+#define LOG_LOCK_INIT(log)	mutex_init(&(log)->loglock)
+#define LOG_LOCK(log)		mutex_lock(&((log)->loglock))
+#define LOG_UNLOCK(log)		mutex_unlock(&((log)->loglock))
+
 /*
  * Log flag
  */
diff --git a/fs/jfs/jfs_umount.c b/fs/jfs/jfs_umount.c
index 8ec43f53f686..18569f1eaabd 100644
--- a/fs/jfs/jfs_umount.c
+++ b/fs/jfs/jfs_umount.c
@@ -20,6 +20,7 @@
 #include "jfs_superblock.h"
 #include "jfs_dmap.h"
 #include "jfs_imap.h"
+#include "jfs_logmgr.h"
 #include "jfs_metapage.h"
 #include "jfs_debug.h"
 
@@ -57,6 +58,12 @@ int jfs_umount(struct super_block *sb)
 		 */
 		jfs_flush_journal(log, 2);
 
+	/*
+	 * Hold log lock so write_special_inodes (lmLogSync) cannot see
+	 * this sbi with a NULL inode pointer while iterating log->sb_list.
+	 */
+	if (log)
+		LOG_LOCK(log);
 	/*
 	 * close fileset inode allocation map (aka fileset inode)
 	 */
@@ -95,6 +102,9 @@ int jfs_umount(struct super_block *sb)
 	 */
 	filemap_write_and_wait(sbi->direct_inode->i_mapping);
 
+	if (log)
+		LOG_UNLOCK(log);
+
 	/*
 	 * ensure all file system file pages are propagated to their
 	 * home blocks on disk (and their in-memory buffer pages are
-- 
2.53.0

Re: [PATCH] jfs: hold LOG_LOCK on umount to avoid null-ptr-deref

Posted by Dave Kleikamp 1 month ago

On 2/27/26 12:11PM, Helen Koike wrote:
> write_special_inodes() function iterate through the log->sb_list and
> access the sbi fields, which can be set to NULL concurrently by umount.
> 
> Fix concurrency issue by holding LOG_LOCK and checking for NULL.
> 
> Reported-by: syzbot+e14b1036481911ae4d77@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e14b1036481911ae4d77
> Signed-off-by: Helen Koike <koike@igalia.com>

Applied to jfs-next.

Thanks!
Shaggy

> ---
>   fs/jfs/jfs_logmgr.c | 16 +++++++---------
>   fs/jfs/jfs_logmgr.h |  7 +++++++
>   fs/jfs/jfs_umount.c | 10 ++++++++++
>   3 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
> index 5b1c5da04163..59f94c28007d 100644
> --- a/fs/jfs/jfs_logmgr.c
> +++ b/fs/jfs/jfs_logmgr.c
> @@ -74,12 +74,6 @@ static struct lbuf *log_redrive_list;
>   static DEFINE_SPINLOCK(log_redrive_lock);
>   
>   
> -/*
> - *	log read/write serialization (per log)
> - */
> -#define LOG_LOCK_INIT(log)	mutex_init(&(log)->loglock)
> -#define LOG_LOCK(log)		mutex_lock(&((log)->loglock))
> -#define LOG_UNLOCK(log)		mutex_unlock(&((log)->loglock))
>   
>   
>   /*
> @@ -204,9 +198,13 @@ static void write_special_inodes(struct jfs_log *log,
>   	struct jfs_sb_info *sbi;
>   
>   	list_for_each_entry(sbi, &log->sb_list, log_list) {
> -		writer(sbi->ipbmap->i_mapping);
> -		writer(sbi->ipimap->i_mapping);
> -		writer(sbi->direct_inode->i_mapping);
> +		/* These pointers can be NULL before list_del during umount */
> +		if (sbi->ipbmap)
> +			writer(sbi->ipbmap->i_mapping);
> +		if (sbi->ipimap)
> +			writer(sbi->ipimap->i_mapping);
> +		if (sbi->direct_inode)
> +			writer(sbi->direct_inode->i_mapping);
>   	}
>   }
>   
> diff --git a/fs/jfs/jfs_logmgr.h b/fs/jfs/jfs_logmgr.h
> index 8b8994e48cd0..09e0ef6aecce 100644
> --- a/fs/jfs/jfs_logmgr.h
> +++ b/fs/jfs/jfs_logmgr.h
> @@ -402,6 +402,13 @@ struct jfs_log {
>   	int no_integrity;	/* 3: flag to disable journaling to disk */
>   };
>   
> +/*
> + * log read/write serialization (per log)
> + */
> +#define LOG_LOCK_INIT(log)	mutex_init(&(log)->loglock)
> +#define LOG_LOCK(log)		mutex_lock(&((log)->loglock))
> +#define LOG_UNLOCK(log)		mutex_unlock(&((log)->loglock))
> +
>   /*
>    * Log flag
>    */
> diff --git a/fs/jfs/jfs_umount.c b/fs/jfs/jfs_umount.c
> index 8ec43f53f686..18569f1eaabd 100644
> --- a/fs/jfs/jfs_umount.c
> +++ b/fs/jfs/jfs_umount.c
> @@ -20,6 +20,7 @@
>   #include "jfs_superblock.h"
>   #include "jfs_dmap.h"
>   #include "jfs_imap.h"
> +#include "jfs_logmgr.h"
>   #include "jfs_metapage.h"
>   #include "jfs_debug.h"
>   
> @@ -57,6 +58,12 @@ int jfs_umount(struct super_block *sb)
>   		 */
>   		jfs_flush_journal(log, 2);
>   
> +	/*
> +	 * Hold log lock so write_special_inodes (lmLogSync) cannot see
> +	 * this sbi with a NULL inode pointer while iterating log->sb_list.
> +	 */
> +	if (log)
> +		LOG_LOCK(log);
>   	/*
>   	 * close fileset inode allocation map (aka fileset inode)
>   	 */
> @@ -95,6 +102,9 @@ int jfs_umount(struct super_block *sb)
>   	 */
>   	filemap_write_and_wait(sbi->direct_inode->i_mapping);
>   
> +	if (log)
> +		LOG_UNLOCK(log);
> +
>   	/*
>   	 * ensure all file system file pages are propagated to their
>   	 * home blocks on disk (and their in-memory buffer pages are