Re: [PATCH v3] loop: Fix NULL pointer dereference in lo_rw_aio()

[Tetsuo Handa]

On 2026/05/30 7:05, Hillf Danton wrote:
>>> Therefore, I think we can address this problem by "drain_workqueue() with disk->open_mutex
>>> held" in the loop driver side.
>>>
>> Good news.
>>
> Bad news: Subject: [syzbot] [block?] possible deadlock in loop_process_work
> [3] https://lore.kernel.org/lkml/6a19f5f7.5099cdd9.8e407.0004.GAE@xxxxxxxxxx/
> 

OK. I sent two patches

  https://lkml.kernel.org/r/147ed056-03d9-4214-b925-0f10fc00cf27@xxxxxxxxxxxxxxxxxxx
  https://lkml.kernel.org/r/148efba2-a0b6-47d7-ac76-b19d2f4b696c@xxxxxxxxxxxxxxxxxxx

as a preparation for evaluating the possibility of calling drain_workqueue() from
__loop_clr_fd(). But as far as syzbot has tested using linux-next tree

  https://syzkaller.appspot.com/bug?extid=c4e9d077bcc86bee08dc
  https://syzkaller.appspot.com/bug?extid=4feabfc9641267769c97

seems to remain even if we applied above patches.

Therefore, I think that we need to call drain_workqueue() from __loop_clr_fd()
without holding disk->open_mutex (if we address this NULL pointer dereference
problem by updating the loop driver).

"[PATCH v3] loop: Fix NULL pointer dereference in lo_rw_aio()" was an attempt to call
drain_workqueue() from __loop_clr_fd() without holding disk->open_mutex, but Sashiko's
review ( https://sashiko.dev/#/patchset/fda8abc8-6aa2-463b-bf72-865f6b838034%40I-love.SAKURA.ne.jp )
mentioned that the "module_put(THIS_MODULE);" executed as the last step of __loop_clr_fd() has
a race window of concurrently triggering module unload operation because module refcount of
the loop driver can become 0 due to this module_put(THIS_MODULE) call. In other words,
we cannot safely manage refcount of the loop module without a support by the caller of
lo_release() (i.e. bdev_release()).

  void bdev_release(struct file *bdev_file)
  {
  (...snipped...)
  	if (bdev_is_partition(bdev))
  		blkdev_put_part(bdev);
  	else
  		blkdev_put_whole(bdev);
  	mutex_unlock(&disk->open_mutex); // <= Keeping holding disk->open_mutex until __loop_clr_fd() completes causes circular locking problem.
  
  	module_put(disk->fops->owner); // <= Calling after __loop_clr_fd() completed is required for managing module refcount safely.
  put_no_open:
  	blkdev_put_no_open(bdev);
  }

Therefore, I think that the only robust and safe approach is, although you won't be
happy to see layering violation / tricky code, either

  (a) allow __loop_clr_fd() to temporarily drop disk->open_mutex

or

  (b) add a new callback for the loop driver which is called between mutex_unlock(&disk->open_mutex) and module_put(disk->fops->owner)

. Jens, what do you think?

One might argue that this problem should be fixed on the filesystem side by
ensuring all filesystems wait for I/O requests safely. However, from the
perspective of defensive programming, the loop driver should be robust enough
to handle incomplete I/O serialization from underlying layers to prevent GPF.
Furthermore, without adding noisy debug printk() messages, it is extremely
difficult to pinpoint which specific layer or filesystem failed to wait for
the I/O requests.