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

[sashiko-bot@kernel.org]

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