Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock order reversal between "ufs" (vnode lock) and vm_map lock in exec #1566

Closed
bsdjhb opened this issue Nov 28, 2022 · 1 comment · Fixed by #1590
Closed

Lock order reversal between "ufs" (vnode lock) and vm_map lock in exec #1566

bsdjhb opened this issue Nov 28, 2022 · 1 comment · Fixed by #1590

Comments

@bsdjhb
Copy link
Collaborator

bsdjhb commented Nov 28, 2022

This is fallout from the reservations changes. Without co-processes this warning is harmless since the vm_map lock is from a new vmspace that isn't accessible to any other threads. With co-processes it's a potential deadlock. If nothing else silencing the warning (which triggers on boot) will avoid scaring users unnecessarily.

Fixes can either be to lock the vm_map "sooner" in exec, or perhaps to use an extra reference on the vnode to permit dropping the vnode lock during exec.

Starting file system checks:
/dev/ufs/root: FILE SYSTEM CLEAN; SKIPPING CHECKS
/dev/ufs/root: clean, 704661 free (5 frags, 88082 blocks, 0.0% fragmentation)
lock order reversal:
 1st 0xffffffd0014ee928 vm map (user) (vm map (user), sx) @ /usr/local/google/home/alexrichardson/cheri/cheribsd/sys/vm/vm_mmap.c:1028
 2nd 0xffffffd0030ad230 ufs (ufs, lockmgr) @ /usr/local/google/home/alexrichardson/cheri/cheribsd/sys/kern/vfs_subr.c:3269
lock order ufs -> vm map (user) established at:
#0 0xffffffc00037d2c0 at witness_checkorder+0xd98
#1 0xffffffc00037c80c at witness_checkorder+0x2e4
#2 0xffffffc000320308 at _sx_xlock+0x60
#3 0xffffffc00057eb86 at vm_map_reservation_create+0x7c
#4 0xffffffc0002a5ba6 at elf64c_populate_note+0x160c
#5 0xffffffc0002a55ee at elf64c_populate_note+0x1054
#6 0xffffffc0002ce036 at kern_execve+0x6a2
#7 0xffffffc0002acebe at mi_startup+0x11ac
#8 0xffffffc0002d6184 at fork_exit+0x68
#9 0xffffffc0005c21da at fork_trampoline+0xa
lock order vm map (user) -> ufs attempted at:
#0 0xffffffc00037cf40 at witness_checkorder+0xa18
#1 0xffffffc0002ea63a at lockmgr_lock_flags+0x116
#2 0xffffffc00054fec6 at ffs_own_mount+0x398e
#3 0xffffffc0005c98c8 at VOP_LOCK1_APV+0x20
#4 0xffffffc0003ff1d0 at _vn_lock+0x4e
#5 0xffffffc0003eb5ba at vlazy+0x282
#6 0xffffffc0003eaf6a at vrele+0x2a
#7 0xffffffc000582d9c at vm_object_deallocate+0x354
#8 0xffffffc0005789e6 at vm_map_delete+0x1d6
#9 0xffffffc00057ca30 at vm_map_remove_locked+0x6c
#10 0xffffffc000580bc0 at kern_munmap+0x6c
#11 0xffffffc000580b44 at sys_munmap+0x6a
#12 0xffffffc0005c2fbe at do_trap_user+0x27c
#13 0xffffffc0005b153e at cpu_exception_handler_user+0xde
Mounting local filesystems:.
@bsdjhb
Copy link
Collaborator Author

bsdjhb commented Dec 14, 2022

Humm, upstream FreeBSD already does the vnode -> vm_map lock order in exec, and specifically in elfN(load_sections).

So the LOR is on the other side (vm_map_delete) and in changes we made for reservations. Upstream FreeBSD is careful to defer the deletion of user map entries via the curthread->td_map_def_user to avoid this LOR. We should probably be doing the same for the vm_object_deallocate we are performing inline now. I wonder if what we really want to be doing is avoid trying to reuse the existing VM map entry that we currently clean, but instead just allocate a brand new one that we insert into the map in place of the old one. This would let us just use vm_map_entry_delete always as with upstream and would be simpler I think. I also worry that there are some steps such as dealing with write counts, etc. that vm_map_process_deferred does that we are missing in our current path.

@bsdjhb bsdjhb linked a pull request Dec 15, 2022 that will close this issue
@bsdjhb bsdjhb closed this as completed Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant