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