On Mon, Nov 28, 2022 at 02:56:17PM -0400, Jason Gunthorpe wrote:

It is basically saying a driver cannot write this:

unmap():
  mutex_lock(lock)
   iommufd_access_unpin_pages(access)
  mutex_unlock(lock)

driver_close
  mutex_lock(lock)
   iommufd_access_destroy(access)
  mutex_unlock(lock)

Or any other equivalent thing. How about

 * iommufd_access_destroy() will wait for any outstanding unmap callback to
 * complete. Once iommufd_access_destroy() no unmap ops are running or will
 * run in the future. Due to this a driver must not create locking that prevents
 * unmap to complete while iommufd_access_destroy() is running.

And I should really add a lockdep map here, which I will add as a
followup patch:

Actually, it turns out we already have enough real locks that lockdep
warns on this anyhow:

[  186.281328] ======================================================
[  186.281647] WARNING: possible circular locking dependency detected
[  186.281930] 6.1.0-rc7+ #156 Not tainted
[  186.282104] ------------------------------------------------------
[  186.282394] iommufd/404 is trying to acquire lock:
[  186.282622] ffff888006e57278 (&staccess->lock){+.+.}-{3:3}, at: iommufd_test_access_unmap+0x2a/0x170 [iommufd]
[  186.283211] 
[  186.283211] but task is already holding lock:
[  186.283498] ffff888008059a70 (&obj->destroy_rwsem){++++}-{3:3}, at: iommufd_access_notify_unmap+0xb7/0x240 [iommufd]
[  186.284000] 
[  186.284000] which lock already depends on the new lock.
[  186.284000] 
[  186.284496] 
[  186.284496] the existing dependency chain (in reverse order) is:
[  186.284905] 
[  186.284905] -> #1 (&obj->destroy_rwsem){++++}-{3:3}:
[  186.285234]        down_write+0x34/0x50
[  186.285438]        iommufd_object_destroy_user+0x1b/0x120 [iommufd]
[  186.285771]        iommufd_access_destroy+0x80/0xa0 [iommufd]
[  186.286111]        iommufd_test_staccess_release+0x5a/0x80 [iommufd]
[  186.286454]        __fput+0x1f9/0x3f0
[  186.286650]        ____fput+0x9/0x10
[  186.286834]        task_work_run+0xf4/0x150
[  186.287026]        exit_to_user_mode_loop+0xd0/0xf0
[  186.287271]        exit_to_user_mode_prepare+0x7f/0xc0
[  186.287519]        syscall_exit_to_user_mode+0x4d/0x1e0
[  186.287768]        do_syscall_64+0x50/0x90
[  186.287958]        entry_SYSCALL_64_after_hwframe+0x46/0xb0
[  186.288206] 
[  186.288206] -> #0 (&staccess->lock){+.+.}-{3:3}:
[  186.288598]        __lock_acquire+0x2092/0x3c80
[  186.288837]        lock_acquire+0x1b5/0x300
[  186.289037]        __mutex_lock_common+0xf7/0x1410
[  186.289299]        mutex_lock_nested+0x1b/0x30
[  186.289561]        iommufd_test_access_unmap+0x2a/0x170 [iommufd]
[  186.289892]        iommufd_access_notify_unmap+0x196/0x240 [iommufd]
[  186.290259]        iopt_unmap_iova_range+0x2c2/0x350 [iommufd]
[  186.290604]        iopt_unmap_iova+0x1b/0x30 [iommufd]
[  186.290889]        iommufd_ioas_unmap+0xdc/0x1d0 [iommufd]
[  186.291170]        iommufd_fops_ioctl+0x1e7/0x210 [iommufd]
[  186.291450]        __x64_sys_ioctl+0x11bb/0x1260
[  186.291707]        do_syscall_64+0x44/0x90
[  186.291903]        entry_SYSCALL_64_after_hwframe+0x46/0xb0

eg trying to provoke it by deliberately wrongly locking
iommufd_access_destroy()

Jason