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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=6a680377e13041c19d50
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR