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

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

On 8/24/26 9:45 PM, Bart Van Assche wrote:
> On 8/23/26 11:12 PM, Nilay Shroff wrote:
>> 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.
> 
> Hi Nilay,
> 
> That's an interesting question. I think we still need
> loop_validate_mutex. Without that mutex the hierarchy could be
> changed from linear into recursive after loop_validate_file()
> has verified the hierarchy and before the loop fd is changed.
> Removing loop_validate_mutex might introduce other race conditions
> than the one mentioned above.
> 
Okay, that makes sense. So it looks like we can't easily get rid
of loop_validate_mutex. In that case, would it make sense to always
acquire loop_validate_mutex rather than acquiring it conditionally
only when the new backing file is a loop device?

I think loop_configure() and loop_change_fd() are control-plane
operations and are relatively infrequent, so taking
loop_validate_mutex should not add any noticeable overhead.
In return, we would have a single locking hierarchy and
the locking-context annotations would be much simpler and easier
to reason about.

What do you think?

Thanks,
--Nilay