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
