Re: [PATCH 07/13] loop: Fix race conditions in loop_validate_file()

[Nilay Shroff <nilay@linux.ibm.com>]

On 8/21/26 1:27 AM, Bart Van Assche wrote:
> Fix race conditions in loop_validate_file() by adding reference counting
> to the file chain traversal.
> 
> Ensure the file reference is kept alive during all dereferences by
> calling get_file() before the loop and deferring fput() until after we
> have locked the target device's lo_mutex and confirmed it is in the
> Lo_bound state.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/block/loop.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c5f026520836..8b633ea6e72f 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -520,7 +520,7 @@ static struct file *loop_get_backing_file(struct loop_device *lo)
>   	 * loop_configure().
>   	 */
>   	rmb();
> -	return lo->lo_backing_file;
> +	return get_file(lo->lo_backing_file);
>   }
>   
>   /* Returns 0 if and only if @file is not backed by loop device @bdev. */
> @@ -534,21 +534,27 @@ static int loop_validate_file(struct loop_device *lo, struct file *file,
>   	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
>   		return -EINVAL;
>   
> +	get_file(f);
>   	/* Avoid recursion */
>   	while (is_loop_device(f)) {
>   		struct loop_device *l;
> +		struct file *prev_f = f;
>   		struct block_device *f_bdev = loop_get_bdev(f);
>   
>   		lockdep_assert_held(&loop_validate_mutex);
> -		if (f_bdev->bd_disk == bdev->bd_disk)
> +		if (f_bdev->bd_disk == bdev->bd_disk) {
> +			fput(f);
>   			return -EBADF;
> +		}
>   
>   		l = f_bdev->bd_disk->private_data;
>   		scoped_guard(mutex, &l->lo_mutex)
>   			f = loop_get_backing_file(l);
> +		fput(prev_f);
>   		if (!f)
>   			return -EINVAL;
>   	}
> +	fput(f);
>   	return 0;
>   }
>   
I see that with the refactoring and the changes in this patch, where we now
take an explicit reference to each backing file while traversing the loop-device
chain and hold the corresponding lo_mutex while checking lo_state and acquiring
that reference, loop_validate_mutex may no longer be necessary.

In particular, loop_get_backing_file() now atomically checks that the loop device
is in Lo_bound state and takes a reference to lo_backing_file while holding
lo_mutex. Therefore, if loop_clr_fd() or loop_change_fd() concurrently replaces
or clears the backing file, the validator still holds its own reference. It also
appears that loop_change_fd() and loop_clr_fd() for the same loop device are
already serialized by lo_mutex.

So I am wondering whether loop_validate_mutex now be redundant? If so, it may
be worth consider removing it as part of this series. That would simplify the
locking and make the context annotations considerably cleaner as well.

Thanks,
--Nilay