[PATCH] udf: avoid recursive s_alloc_mutex deadlock when freeing AED blocks

[PATCH] udf: avoid recursive s_alloc_mutex deadlock when freeing AED blocks

[Deepanshu Kartikey]

udf_table_prealloc_blocks() and udf_table_new_block() call
udf_delete_aext() on the unallocated-space-table inode while holding
sbi->s_alloc_mutex. When the deleted allocation descriptor empties an
allocation-extent (AED) block, udf_delete_aext() returns that block to
free space via udf_free_blocks(). For a table-managed partition that
path is udf_free_blocks() -> udf_table_free_blocks() ->
mutex_lock(&sbi->s_alloc_mutex), i.e. the task tries to acquire a mutex
it already holds.

On a PREEMPT_RT kernel the rtmutex deadlock detector reports this as
-EDEADLK:

  rtmutex deadlock detected
  WARNING: kernel/locking/rtmutex.c:1698 at rt_mutex_handle_deadlock
  udf_table_free_blocks fs/udf/balloc.c:376 [inline]
  udf_free_blocks+0xa8c/0x1900 fs/udf/balloc.c:678
  udf_delete_aext+0x4f5/0xc00 fs/udf/inode.c:2381
  udf_table_prealloc_blocks fs/udf/balloc.c:544 [inline]
  udf_prealloc_blocks+0xbd4/0x10e0 fs/udf/balloc.c:702

On a non-RT kernel the same path is a hard self-deadlock (or a lockdep
recursive-locking splat). It is reachable from a crafted UDF image via
the write/sendfile path.

The allocator already refuses to recurse on the add side: see the
comment in udf_table_free_blocks() explaining why it must not call
udf_add_aext() while holding s_alloc_mutex. Apply the same rule to the
delete side. Let udf_delete_aext() report the AED block it would
otherwise free through a new out-parameter, and have the two callers
that hold s_alloc_mutex free it after dropping the lock. The block is
already unlinked from the inode's descriptor chain by then, so nothing
can reference it in the meantime. All other callers pass NULL and keep
freeing the block inline, exactly as before.

The out-parameter is pre-initialised with the reserved partition number
0xFFFF, which can never name a real block, so the caller can tell
whether a block was handed back.

Reported-by: syzbot+6a680377e13041c19d50@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6a680377e13041c19d50
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 fs/udf/balloc.c   | 12 ++++++++++--
 fs/udf/inode.c    | 19 ++++++++++++++++---
 fs/udf/truncate.c |  2 +-
 fs/udf/udfdecl.h  |  3 ++-
 4 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index cc6dc6e1d84d..30cec5600149 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -502,6 +502,8 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
 	int8_t etype = -1;
 	struct udf_inode_info *iinfo;
 	int ret = 0;
+	/* AED block freed by udf_delete_aext(), released after unlock */
+	struct kernel_lb_addr freed = { .partitionReferenceNum = 0xFFFF };
 
 	if (first_block >= sbi->s_partmaps[partition].s_partition_len)
 		return 0;
@@ -541,7 +543,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
 			udf_write_aext(table, &epos, &eloc,
 					(etype << 30) | elen, 1);
 		} else
-			udf_delete_aext(table, epos);
+			udf_delete_aext(table, epos, &freed);
 	} else {
 		alloc_count = 0;
 	}
@@ -552,6 +554,8 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
 	if (alloc_count)
 		udf_add_free_space(sb, partition, -alloc_count);
 	mutex_unlock(&sbi->s_alloc_mutex);
+	if (freed.partitionReferenceNum != 0xFFFF)
+		udf_free_blocks(sb, table, &freed, 0, 1);
 	return alloc_count;
 }
 
@@ -560,6 +564,8 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
 			       uint32_t goal, int *err)
 {
 	struct udf_sb_info *sbi = UDF_SB(sb);
+	/* AED block freed by udf_delete_aext(), released after unlock */
+	struct kernel_lb_addr freed = { .partitionReferenceNum = 0xFFFF };
 	uint32_t spread = 0xFFFFFFFF, nspread = 0xFFFFFFFF;
 	udf_pblk_t newblock = 0;
 	uint32_t adsize;
@@ -643,12 +649,14 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
 	if (goal_elen)
 		udf_write_aext(table, &goal_epos, &goal_eloc, goal_elen, 1);
 	else
-		udf_delete_aext(table, goal_epos);
+		udf_delete_aext(table, goal_epos, &freed);
 	brelse(goal_epos.bh);
 
 	udf_add_free_space(sb, partition, -1);
 
 	mutex_unlock(&sbi->s_alloc_mutex);
+	if (freed.partitionReferenceNum != 0xFFFF)
+		udf_free_blocks(sb, table, &freed, 0, 1);
 	*err = 0;
 	return newblock;
 }
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 67bcf83758c8..ebb67ce0aed7 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1204,7 +1204,7 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
 
 	if (startnum > endnum) {
 		for (i = 0; i < (startnum - endnum); i++)
-			udf_delete_aext(inode, *epos);
+			udf_delete_aext(inode, *epos, NULL);
 	} else if (startnum < endnum) {
 		for (i = 0; i < (endnum - startnum); i++) {
 			err = udf_insert_aext(inode, *epos,
@@ -2328,7 +2328,8 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
 	return ret;
 }
 
-int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
+int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
+			struct kernel_lb_addr *freed)
 {
 	struct extent_position oepos;
 	int adsize;
@@ -2378,7 +2379,19 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
 	elen = 0;
 
 	if (epos.bh != oepos.bh) {
-		udf_free_blocks(inode->i_sb, inode, &epos.block, 0, 1);
+		/*
+		 * The block that held the now-empty allocation extent must be
+		 * returned to free space.  When the caller already holds
+		 * s_alloc_mutex (the space-table allocator in balloc.c),
+		 * freeing it inline would recurse through udf_free_blocks()
+		 * into udf_table_free_blocks() and deadlock re-acquiring
+		 * s_alloc_mutex.  In that case report the block to the caller,
+		 *  which frees it after dropping the lock.
+		 */
+		if (freed)
+			*freed = epos.block;
+		else
+			udf_free_blocks(inode->i_sb, inode, &epos.block, 0, 1);
 		udf_write_aext(inode, &oepos, &eloc, elen, 1);
 		udf_write_aext(inode, &oepos, &eloc, elen, 1);
 		if (!oepos.bh) {
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index 41b2bfd30449..0990f94b8551 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -159,7 +159,7 @@ void udf_discard_prealloc(struct inode *inode)
 
 	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
 		lbcount -= elen;
-		udf_delete_aext(inode, prev_epos);
+		udf_delete_aext(inode, prev_epos, NULL);
 		udf_free_blocks(inode->i_sb, inode, &eloc, 0,
 				DIV_ROUND_UP(elen, bsize));
 	}
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 6d951e05c004..01e4ce8644a9 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -170,7 +170,8 @@ extern int udf_add_aext(struct inode *, struct extent_position *,
 			struct kernel_lb_addr *, uint32_t, int);
 extern void udf_write_aext(struct inode *, struct extent_position *,
 			   struct kernel_lb_addr *, uint32_t, int);
-extern int8_t udf_delete_aext(struct inode *, struct extent_position);
+extern int8_t udf_delete_aext(struct inode *, struct extent_position,
+			       struct kernel_lb_addr *);
 extern int udf_next_aext(struct inode *inode, struct extent_position *epos,
 			 struct kernel_lb_addr *eloc, uint32_t *elen,
 			 int8_t *etype, int inc);
-- 
2.43.0

Re: [PATCH] udf: avoid recursive s_alloc_mutex deadlock when freeing AED blocks

[sashiko-bot]

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

Pre-existing issues:
- [High] udf_table_prealloc_blocks() and udf_table_new_block() ignore the return value of udf_delete_aext(), leading to double-allocation on error.
- [High] udf_delete_aext() leaks two buffer_head references on early return.
--

commit b35154de447cbd2ab36c556abd7be97822502a4b
Author: Deepanshu Kartikey <kartikey406@gmail.com>

udf: avoid recursive s_alloc_mutex deadlock when freeing AED blocks

This commit fixes a recursive deadlock on s_alloc_mutex by modifying
udf_delete_aext() to return the freed block via an out-parameter.
Callers can then free the block after dropping the mutex.

Link: https://syzkaller.appspot.com/bug?extid=6a680377e13041c19d50

> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index cc6dc6e1d84de..30cec56001491 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c

[ ... ]

> @@ -541,7 +543,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
>  			udf_write_aext(table, &epos, &eloc,
>  					(etype << 30) | elen, 1);
>  		} else
> -			udf_delete_aext(table, epos);
> +			udf_delete_aext(table, epos, &freed);

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
udf_delete_aext() here risk a double-allocation?

If udf_delete_aext() returns -1 due to an error, the block remains in
the unallocated space table. Since udf_table_prealloc_blocks() proceeds
as if the deletion succeeded, could a subsequent allocation find and
allocate the exact same block?

[ ... ]

> @@ -643,12 +649,14 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
>  	if (goal_elen)
>  		udf_write_aext(table, &goal_epos, &goal_eloc, goal_elen, 1);
>  	else
> -		udf_delete_aext(table, goal_epos);
> +		udf_delete_aext(table, goal_epos, &freed);

[Severity: High]
This is a pre-existing issue, but the same return value behavior happens
here in udf_table_new_block() as well. Does this code need to handle
the case where udf_delete_aext() returns an error to avoid corruption?

> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 67bcf83758c8f..2c8647e0c9131 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c

[ ... ]

> @@ -2328,7 +2328,8 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>  	return ret;
>  }
>  
> -int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
> +int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
> +			struct kernel_lb_addr *freed)
>  {
>  	struct extent_position oepos;
>  	int adsize;

[Severity: High]
This is a pre-existing issue, but does the early return path in this
function leak buffer_head references?

At the start of udf_delete_aext(), two references to epos.bh are acquired:

if (epos.bh) {
        get_bh(epos.bh);
        get_bh(epos.bh);
}

However, if the initial extent traversal fails shortly after:

if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1) <= 0)
        return -1;

This early return path does not appear to call brelse() to release the
acquired references, which could pin the buffer in memory and prevent
unmounting.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621035841.56194-1-kartikey406@gmail.com?part=1

Re: [PATCH] udf: avoid recursive s_alloc_mutex deadlock when freeing AED blocks

[Jan Kara]

On Sun 21-06-26 09:28:41, Deepanshu Kartikey wrote:
> udf_table_prealloc_blocks() and udf_table_new_block() call
> udf_delete_aext() on the unallocated-space-table inode while holding
> sbi->s_alloc_mutex. When the deleted allocation descriptor empties an
> allocation-extent (AED) block, udf_delete_aext() returns that block to
> free space via udf_free_blocks(). For a table-managed partition that
> path is udf_free_blocks() -> udf_table_free_blocks() ->
> mutex_lock(&sbi->s_alloc_mutex), i.e. the task tries to acquire a mutex
> it already holds.
> 
> On a PREEMPT_RT kernel the rtmutex deadlock detector reports this as
> -EDEADLK:
> 
>   rtmutex deadlock detected
>   WARNING: kernel/locking/rtmutex.c:1698 at rt_mutex_handle_deadlock
>   udf_table_free_blocks fs/udf/balloc.c:376 [inline]
>   udf_free_blocks+0xa8c/0x1900 fs/udf/balloc.c:678
>   udf_delete_aext+0x4f5/0xc00 fs/udf/inode.c:2381
>   udf_table_prealloc_blocks fs/udf/balloc.c:544 [inline]
>   udf_prealloc_blocks+0xbd4/0x10e0 fs/udf/balloc.c:702
> 
> On a non-RT kernel the same path is a hard self-deadlock (or a lockdep
> recursive-locking splat). It is reachable from a crafted UDF image via
> the write/sendfile path.
> 
> The allocator already refuses to recurse on the add side: see the
> comment in udf_table_free_blocks() explaining why it must not call
> udf_add_aext() while holding s_alloc_mutex. Apply the same rule to the
> delete side. Let udf_delete_aext() report the AED block it would
> otherwise free through a new out-parameter, and have the two callers
> that hold s_alloc_mutex free it after dropping the lock. The block is
> already unlinked from the inode's descriptor chain by then, so nothing
> can reference it in the meantime. All other callers pass NULL and keep
> freeing the block inline, exactly as before.
> 
> The out-parameter is pre-initialised with the reserved partition number
> 0xFFFF, which can never name a real block, so the caller can tell
> whether a block was handed back.
> 
> Reported-by: syzbot+6a680377e13041c19d50@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6a680377e13041c19d50
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>

Thanks! I've added the patch to my tree.

								Honza

> ---
>  fs/udf/balloc.c   | 12 ++++++++++--
>  fs/udf/inode.c    | 19 ++++++++++++++++---
>  fs/udf/truncate.c |  2 +-
>  fs/udf/udfdecl.h  |  3 ++-
>  4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index cc6dc6e1d84d..30cec5600149 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -502,6 +502,8 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
>  	int8_t etype = -1;
>  	struct udf_inode_info *iinfo;
>  	int ret = 0;
> +	/* AED block freed by udf_delete_aext(), released after unlock */
> +	struct kernel_lb_addr freed = { .partitionReferenceNum = 0xFFFF };
>  
>  	if (first_block >= sbi->s_partmaps[partition].s_partition_len)
>  		return 0;
> @@ -541,7 +543,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
>  			udf_write_aext(table, &epos, &eloc,
>  					(etype << 30) | elen, 1);
>  		} else
> -			udf_delete_aext(table, epos);
> +			udf_delete_aext(table, epos, &freed);
>  	} else {
>  		alloc_count = 0;
>  	}
> @@ -552,6 +554,8 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
>  	if (alloc_count)
>  		udf_add_free_space(sb, partition, -alloc_count);
>  	mutex_unlock(&sbi->s_alloc_mutex);
> +	if (freed.partitionReferenceNum != 0xFFFF)
> +		udf_free_blocks(sb, table, &freed, 0, 1);
>  	return alloc_count;
>  }
>  
> @@ -560,6 +564,8 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
>  			       uint32_t goal, int *err)
>  {
>  	struct udf_sb_info *sbi = UDF_SB(sb);
> +	/* AED block freed by udf_delete_aext(), released after unlock */
> +	struct kernel_lb_addr freed = { .partitionReferenceNum = 0xFFFF };
>  	uint32_t spread = 0xFFFFFFFF, nspread = 0xFFFFFFFF;
>  	udf_pblk_t newblock = 0;
>  	uint32_t adsize;
> @@ -643,12 +649,14 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
>  	if (goal_elen)
>  		udf_write_aext(table, &goal_epos, &goal_eloc, goal_elen, 1);
>  	else
> -		udf_delete_aext(table, goal_epos);
> +		udf_delete_aext(table, goal_epos, &freed);
>  	brelse(goal_epos.bh);
>  
>  	udf_add_free_space(sb, partition, -1);
>  
>  	mutex_unlock(&sbi->s_alloc_mutex);
> +	if (freed.partitionReferenceNum != 0xFFFF)
> +		udf_free_blocks(sb, table, &freed, 0, 1);
>  	*err = 0;
>  	return newblock;
>  }
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 67bcf83758c8..ebb67ce0aed7 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1204,7 +1204,7 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>  
>  	if (startnum > endnum) {
>  		for (i = 0; i < (startnum - endnum); i++)
> -			udf_delete_aext(inode, *epos);
> +			udf_delete_aext(inode, *epos, NULL);
>  	} else if (startnum < endnum) {
>  		for (i = 0; i < (endnum - startnum); i++) {
>  			err = udf_insert_aext(inode, *epos,
> @@ -2328,7 +2328,8 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>  	return ret;
>  }
>  
> -int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
> +int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
> +			struct kernel_lb_addr *freed)
>  {
>  	struct extent_position oepos;
>  	int adsize;
> @@ -2378,7 +2379,19 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>  	elen = 0;
>  
>  	if (epos.bh != oepos.bh) {
> -		udf_free_blocks(inode->i_sb, inode, &epos.block, 0, 1);
> +		/*
> +		 * The block that held the now-empty allocation extent must be
> +		 * returned to free space.  When the caller already holds
> +		 * s_alloc_mutex (the space-table allocator in balloc.c),
> +		 * freeing it inline would recurse through udf_free_blocks()
> +		 * into udf_table_free_blocks() and deadlock re-acquiring
> +		 * s_alloc_mutex.  In that case report the block to the caller,
> +		 *  which frees it after dropping the lock.
> +		 */
> +		if (freed)
> +			*freed = epos.block;
> +		else
> +			udf_free_blocks(inode->i_sb, inode, &epos.block, 0, 1);
>  		udf_write_aext(inode, &oepos, &eloc, elen, 1);
>  		udf_write_aext(inode, &oepos, &eloc, elen, 1);
>  		if (!oepos.bh) {
> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> index 41b2bfd30449..0990f94b8551 100644
> --- a/fs/udf/truncate.c
> +++ b/fs/udf/truncate.c
> @@ -159,7 +159,7 @@ void udf_discard_prealloc(struct inode *inode)
>  
>  	if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
>  		lbcount -= elen;
> -		udf_delete_aext(inode, prev_epos);
> +		udf_delete_aext(inode, prev_epos, NULL);
>  		udf_free_blocks(inode->i_sb, inode, &eloc, 0,
>  				DIV_ROUND_UP(elen, bsize));
>  	}
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 6d951e05c004..01e4ce8644a9 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -170,7 +170,8 @@ extern int udf_add_aext(struct inode *, struct extent_position *,
>  			struct kernel_lb_addr *, uint32_t, int);
>  extern void udf_write_aext(struct inode *, struct extent_position *,
>  			   struct kernel_lb_addr *, uint32_t, int);
> -extern int8_t udf_delete_aext(struct inode *, struct extent_position);
> +extern int8_t udf_delete_aext(struct inode *, struct extent_position,
> +			       struct kernel_lb_addr *);
>  extern int udf_next_aext(struct inode *inode, struct extent_position *epos,
>  			 struct kernel_lb_addr *eloc, uint32_t *elen,
>  			 int8_t *etype, int inc);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR