jfs: Fix use-after-free read issue in jfs_lazycommit

[PATCH] jfs: Fix use-after-free read issue in jfs_lazycommit

Posted by Qianqiang Liu 1 year, 2 months ago

The jfsCommit kernel thread uses the sbi->commit_state flag,
and sbi may be freed in jfs_put_super() by another thread.

To prevent this, move commit_state to struct tblock,
eliminating the need to access the sbi variable.

Reported-by: syzbot+885a4f3281b8d99c48d8@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=885a4f3281b8d99c48d8
Tested-by: syzbot+885a4f3281b8d99c48d8@syzkaller.appspotmail.com
Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
---
 fs/jfs/jfs_incore.h |  8 --------
 fs/jfs/jfs_txnmgr.c | 10 ++++------
 fs/jfs/jfs_txnmgr.h |  8 ++++++++
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
index 10934f9a11be..7b75c801b239 100644
--- a/fs/jfs/jfs_incore.h
+++ b/fs/jfs/jfs_incore.h
@@ -177,11 +177,6 @@ struct jfs_sb_info {
 	pxd_t		ait2;		/* pxd describing AIT copy	*/
 	uuid_t		uuid;		/* 128-bit uuid for volume	*/
 	uuid_t		loguuid;	/* 128-bit uuid for log	*/
-	/*
-	 * commit_state is used for synchronization of the jfs_commit
-	 * threads.  It is protected by LAZY_LOCK().
-	 */
-	int		commit_state;	/* commit state */
 	/* Formerly in ipimap */
 	uint		gengen;		/* inode generation generator*/
 	uint		inostamp;	/* shows inode belongs to fileset*/
@@ -199,9 +194,6 @@ struct jfs_sb_info {
 	uint		minblks_trim;	/* minimum blocks, for online trim */
 };
 
-/* jfs_sb_info commit_state */
-#define IN_LAZYCOMMIT 1
-
 static inline struct jfs_inode_info *JFS_IP(struct inode *inode)
 {
 	return container_of(inode, struct jfs_inode_info, vfs_inode);
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index be17e3c43582..a4817229d573 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -2700,7 +2700,6 @@ int jfs_lazycommit(void *arg)
 	int WorkDone;
 	struct tblock *tblk;
 	unsigned long flags;
-	struct jfs_sb_info *sbi;
 
 	set_freezable();
 	do {
@@ -2711,17 +2710,16 @@ int jfs_lazycommit(void *arg)
 			list_for_each_entry(tblk, &TxAnchor.unlock_queue,
 					    cqueue) {
 
-				sbi = JFS_SBI(tblk->sb);
 				/*
 				 * For each volume, the transactions must be
 				 * handled in order.  If another commit thread
 				 * is handling a tblk for this superblock,
 				 * skip it
 				 */
-				if (sbi->commit_state & IN_LAZYCOMMIT)
+				if (tblk->commit_state & IN_LAZYCOMMIT)
 					continue;
 
-				sbi->commit_state |= IN_LAZYCOMMIT;
+				tblk->commit_state |= IN_LAZYCOMMIT;
 				WorkDone = 1;
 
 				/*
@@ -2733,7 +2731,7 @@ int jfs_lazycommit(void *arg)
 				txLazyCommit(tblk);
 				LAZY_LOCK(flags);
 
-				sbi->commit_state &= ~IN_LAZYCOMMIT;
+				tblk->commit_state &= ~IN_LAZYCOMMIT;
 				/*
 				 * Don't continue in the for loop.  (We can't
 				 * anyway, it's unsafe!)  We want to go back to
@@ -2781,7 +2779,7 @@ void txLazyUnlock(struct tblock * tblk)
 	 * Don't wake up a commit thread if there is already one servicing
 	 * this superblock, or if the last one we woke up hasn't started yet.
 	 */
-	if (!(JFS_SBI(tblk->sb)->commit_state & IN_LAZYCOMMIT) &&
+	if (!(tblk->commit_state & IN_LAZYCOMMIT) &&
 	    !jfs_commit_thread_waking) {
 		jfs_commit_thread_waking = 1;
 		wake_up(&jfs_commit_thread_wait);
diff --git a/fs/jfs/jfs_txnmgr.h b/fs/jfs/jfs_txnmgr.h
index ba71eb5ced56..3a0ee53f17cb 100644
--- a/fs/jfs/jfs_txnmgr.h
+++ b/fs/jfs/jfs_txnmgr.h
@@ -32,6 +32,11 @@ struct tblock {
 
 	/* lock management */
 	struct super_block *sb;	/* super block */
+	/*
+	 * commit_state is used for synchronization of the jfs_commit
+	 * threads.  It is protected by LAZY_LOCK().
+	 */
+	int commit_state;	/* commit state */
 	lid_t next;		/* index of first tlock of tid */
 	lid_t last;		/* index of last tlock of tid */
 	wait_queue_head_t waitor;	/* tids waiting on this tid */
@@ -56,6 +61,9 @@ struct tblock {
 	u32 ino;		/* inode number being created */
 };
 
+/* tblock commit_state */
+#define IN_LAZYCOMMIT 1
+
 extern struct tblock *TxBlock;	/* transaction block table */
 
 /* commit flags: tblk->xflag */
-- 
2.47.0

Re: [PATCH] jfs: Fix use-after-free read issue in jfs_lazycommit

Posted by Dave Kleikamp 1 year, 1 month ago

On 10/13/24 1:05AM, Qianqiang Liu wrote:
> The jfsCommit kernel thread uses the sbi->commit_state flag,
> and sbi may be freed in jfs_put_super() by another thread.
> 
> To prevent this, move commit_state to struct tblock,
> eliminating the need to access the sbi variable.

I need to give this one some more thought. The unmount isn't supposed to 
complete before all I/O has completed, but it's been quite I while since 
I went over the mechanisms to safeguard that. I'll have to look at this 
problem more closely.

Shaggy

> 
> Reported-by: syzbot+885a4f3281b8d99c48d8@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=885a4f3281b8d99c48d8
> Tested-by: syzbot+885a4f3281b8d99c48d8@syzkaller.appspotmail.com
> Signed-off-by: Qianqiang Liu <qianqiang.liu@163.com>
> ---
>   fs/jfs/jfs_incore.h |  8 --------
>   fs/jfs/jfs_txnmgr.c | 10 ++++------
>   fs/jfs/jfs_txnmgr.h |  8 ++++++++
>   3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
> index 10934f9a11be..7b75c801b239 100644
> --- a/fs/jfs/jfs_incore.h
> +++ b/fs/jfs/jfs_incore.h
> @@ -177,11 +177,6 @@ struct jfs_sb_info {
>   	pxd_t		ait2;		/* pxd describing AIT copy	*/
>   	uuid_t		uuid;		/* 128-bit uuid for volume	*/
>   	uuid_t		loguuid;	/* 128-bit uuid for log	*/
> -	/*
> -	 * commit_state is used for synchronization of the jfs_commit
> -	 * threads.  It is protected by LAZY_LOCK().
> -	 */
> -	int		commit_state;	/* commit state */
>   	/* Formerly in ipimap */
>   	uint		gengen;		/* inode generation generator*/
>   	uint		inostamp;	/* shows inode belongs to fileset*/
> @@ -199,9 +194,6 @@ struct jfs_sb_info {
>   	uint		minblks_trim;	/* minimum blocks, for online trim */
>   };
>   
> -/* jfs_sb_info commit_state */
> -#define IN_LAZYCOMMIT 1
> -
>   static inline struct jfs_inode_info *JFS_IP(struct inode *inode)
>   {
>   	return container_of(inode, struct jfs_inode_info, vfs_inode);
> diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
> index be17e3c43582..a4817229d573 100644
> --- a/fs/jfs/jfs_txnmgr.c
> +++ b/fs/jfs/jfs_txnmgr.c
> @@ -2700,7 +2700,6 @@ int jfs_lazycommit(void *arg)
>   	int WorkDone;
>   	struct tblock *tblk;
>   	unsigned long flags;
> -	struct jfs_sb_info *sbi;
>   
>   	set_freezable();
>   	do {
> @@ -2711,17 +2710,16 @@ int jfs_lazycommit(void *arg)
>   			list_for_each_entry(tblk, &TxAnchor.unlock_queue,
>   					    cqueue) {
>   
> -				sbi = JFS_SBI(tblk->sb);
>   				/*
>   				 * For each volume, the transactions must be
>   				 * handled in order.  If another commit thread
>   				 * is handling a tblk for this superblock,
>   				 * skip it
>   				 */
> -				if (sbi->commit_state & IN_LAZYCOMMIT)
> +				if (tblk->commit_state & IN_LAZYCOMMIT)
>   					continue;
>   
> -				sbi->commit_state |= IN_LAZYCOMMIT;
> +				tblk->commit_state |= IN_LAZYCOMMIT;
>   				WorkDone = 1;
>   
>   				/*
> @@ -2733,7 +2731,7 @@ int jfs_lazycommit(void *arg)
>   				txLazyCommit(tblk);
>   				LAZY_LOCK(flags);
>   
> -				sbi->commit_state &= ~IN_LAZYCOMMIT;
> +				tblk->commit_state &= ~IN_LAZYCOMMIT;
>   				/*
>   				 * Don't continue in the for loop.  (We can't
>   				 * anyway, it's unsafe!)  We want to go back to
> @@ -2781,7 +2779,7 @@ void txLazyUnlock(struct tblock * tblk)
>   	 * Don't wake up a commit thread if there is already one servicing
>   	 * this superblock, or if the last one we woke up hasn't started yet.
>   	 */
> -	if (!(JFS_SBI(tblk->sb)->commit_state & IN_LAZYCOMMIT) &&
> +	if (!(tblk->commit_state & IN_LAZYCOMMIT) &&
>   	    !jfs_commit_thread_waking) {
>   		jfs_commit_thread_waking = 1;
>   		wake_up(&jfs_commit_thread_wait);
> diff --git a/fs/jfs/jfs_txnmgr.h b/fs/jfs/jfs_txnmgr.h
> index ba71eb5ced56..3a0ee53f17cb 100644
> --- a/fs/jfs/jfs_txnmgr.h
> +++ b/fs/jfs/jfs_txnmgr.h
> @@ -32,6 +32,11 @@ struct tblock {
>   
>   	/* lock management */
>   	struct super_block *sb;	/* super block */
> +	/*
> +	 * commit_state is used for synchronization of the jfs_commit
> +	 * threads.  It is protected by LAZY_LOCK().
> +	 */
> +	int commit_state;	/* commit state */
>   	lid_t next;		/* index of first tlock of tid */
>   	lid_t last;		/* index of last tlock of tid */
>   	wait_queue_head_t waitor;	/* tids waiting on this tid */
> @@ -56,6 +61,9 @@ struct tblock {
>   	u32 ino;		/* inode number being created */
>   };
>   
> +/* tblock commit_state */
> +#define IN_LAZYCOMMIT 1
> +
>   extern struct tblock *TxBlock;	/* transaction block table */
>   
>   /* commit flags: tblk->xflag */