Re: [PATCH v2 02/12] loop: Remove the "bool global" function argument

On 8/3/26 11:11 PM, Bart Van Assche wrote:
> On 8/3/26 6:05 AM, Nilay Shroff wrote:
>> Overall this change looks good to me. But I see, lo->lo_lock
>> is used to protect lo->lo_backing_file. so shall we annotate
>> the lo->lo_backing_file using __guarded_by(&lo_lock)?
> 
> I don't think so. It seems to me that the strategy in the loop driver
> for serializing accesses to lo->lo_backing_file is too complicated for
> lock context annotations. My understanding is as follows:
> * lo->lo_mutex serializes configuration and state changes on an
>    individual loop device.
> * loop_validate_mutex serializes concurrent loop_configure(),
>    loop_change_fd(), and loop_clr_fd() calls across all loop devices to
>    safely execute loop_validate_file() when loop devices are stacked
>    or nested.
> * The blk_mq_freeze_queue() call in __loop_change_fd() serializes I/O
>    request processing and the code in __loop_change_fd() that is executed
>    while the queue is frozen.
> 

Yes, you're right about the locking being more complicated here. However,
I think there may be another issue: a race between the sysfs read in
loop_attr_backing_file_show() and replacement of the backing file in
__loop_change_fd().

__loop_change_fd() replaces lo->lo_backing_file through loop_assign_backing_file(),
but loop_assign_backing_file() does not take lo->lo_lock. Hence, the sysfs path
could read the old lo_backing_file pointer while __loop_change_fd() replaces it
and subsequently drops the old file reference with fput().

Since loop_attr_backing_file_show() does not take its own reference to the file,
this looks like it could result in a use-after-free.

Initially, I thought we could fix this by consistently protecting publication/removal
of lo->lo_backing_file with lo->lo_lock and taking a temporary file reference from
the sysfs path, e.g.:

static ssize_t loop_attr_backing_file_show(struct loop_device *lo, char *buf)
{
	struct file *file;
	...

	spin_lock_irq(&lo->lo_lock);
	file = lo->lo_backing_file;
	if (file)
		get_file(file);
	spin_unlock_irq(&lo->lo_lock);

	if (!file)
		return -ENOENT;

	...
	fput(file);
}
```

and similarly protect the assignment in loop_assign_backing_file().

static void loop_assign_backing_file(struct loop_device *lo, struct file *file)
{
         spin_lock_irq(&lo->lo_lock);
         lo->lo_backing_file = file;
         spin_unlock_irq(&lo->lo_lock);
         ...
}

However, looking further through the code, I wonder whether we can instead get rid of
lo->lo_lock entirely and use lo->lo_mutex to serialize the sysfs access to lo->lo_backing_file.

__loop_change_fd() is already called with lo->lo_mutex held, so the update path would not
need any additional locking. We would only need to acquire lo->lo_mutex in
loop_attr_backing_file_show() while accessing lo->lo_backing_file.

Similarly, for the clear path in __loop_clr_fd(), it looks like the existing device teardown
serialization may already be sufficient, in which case the lo->lo_lock protection around
clearing lo->lo_backing_file could potentially be removed as well.

If that's correct, we could simplify the locking and remove lo->lo_lock altogether rather than
adding more users of it.

Thanks,
--Nilay