Page MenuHomeFreeBSD

Rework jobc handling.
ClosedPublic

Authored by kib on Dec 31 2020, 7:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jan 3, 10:50 PM
Unknown Object (File)
Sat, Dec 21, 4:07 PM
Unknown Object (File)
Nov 29 2024, 3:38 AM
Unknown Object (File)
Nov 28 2024, 4:39 PM
Unknown Object (File)
Nov 25 2024, 5:30 PM
Unknown Object (File)
Nov 19 2024, 3:45 AM
Unknown Object (File)
Nov 19 2024, 2:44 AM
Unknown Object (File)
Nov 19 2024, 2:39 AM

Details

Summary

Instead of trying to maintain pg_jobc counter on each process group update (and sometimes before), just calculate the counter when needed. Still, for the benefit of the signal delivery code, explicitly mark orphaned groups as such with the new process group flag.

Fix use after free when relocking p_pgrp in tty_wait_background (and probably other places). Change pgrp memory to type-stable zone, and ensure that pg_mtx is not destroyed.

Not sure how to present it in phab, there https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/jobcdbg is the branch with individual commits.

Tested by: pho

Test Plan

Shells work fine.
I checked things like

# sh
# cat &
# ^D

to deliver expected signals to the stopped orphaned processes.

Also there is tools/regression/sigqueue/sigqtest2

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Dec 31 2020, 7:10 PM
kib created this revision.

Fix LOR between proctree and pidhash bucket lock.
Do not forget un-orphaning groups when reparenting on exit.

FWIW, I upload patch series as a set of arc reviews. Originally this was tedious (using arc diff --create --head <N> <N-1> for each commit), but @markj has been working on a script that automates this (I used it to upload the series of zombproc-related debugger cleanups as a single command from a branch with multiple commits).

sys/kern/kern_proc.c
1404
20210101 07:17:42 all (6/740): machipc2.sh
panic: Lock (sx) proctree not locked @ kern/kern_proc.c:1172.
cpuid = 17
time = 1609481864
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe01c7c16ab0
vpanic() at vpanic+0x181/frame 0xfffffe01c7c16b00
panic() at panic+0x43/frame 0xfffffe01c7c16b60
witness_assert() at witness_assert+0x23a/frame 0xfffffe01c7c16b90
fill_kinfo_proc() at fill_kinfo_proc+0x4e/frame 0xfffffe01c7c16be0
kern_proc_out() at kern_proc_out+0x62/frame 0xfffffe01c7c17370
sysctl_out_proc() at sysctl_out_proc+0x68/frame 0xfffffe01c7c17840
sysctl_kern_proc() at sysctl_kern_proc+0x87/frame 0xfffffe01c7c17890
sysctl_root_handler_locked() at sysctl_root_handler_locked+0x9c/frame 0xfffffe01c7c178e0
sysctl_root() at sysctl_root+0x20d/frame 0xfffffe01c7c17960
userland_sysctl() at userland_sysctl+0x180/frame 0xfffffe01c7c17a10
sys___sysctl() at sys___sysctl+0x5f/frame 0xfffffe01c7c17ac0
amd64_syscall() at amd64_syscall+0x147/frame 0xfffffe01c7c17bf0
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe01c7c17bf0
--- syscall (202, FreeBSD ELF64, sys___sysctl), rip = 0x8003fb9ea, rsp = 0x7fffffffd258, rbp = 0x7fffffffd290 ---
KDB: enter: panic
[ thread pid 78475 tid 792240 ]
Stopped at      kdb_enter+0x37: movq    $0,0x10960c6(%rip)
db>

https://people.freebsd.org/~pho/stress/log/log0014.txt

One more place for proctree_lock.

kinfo_proc_out is also called from ELF coredump code

This revision is now accepted and ready to land.Jan 9 2021, 7:50 PM

I realize this is quite old, but I'd like to discuss the possibility of reverting 5844bd058aed, as it appears to have a significant performance impact which was never discussed at the time, while the benefit of the change appears to be purely hypothetical. I have no reason to doubt that pgrp_calc_jobc() is correct, but my own attempts to investigate possible errors in tracking pg_jobc did not uncover anything except an unrelated race condition in ppoll() and pselect() (cf. D47738 and D47741). I know of historical instances, e.g. D26116, but see no evidence of any unresolved issues at the time. Meanwhile, the cost of pgrp_calc_jobc() is quite high, especially in a debugging kernel where the process tree lock is being asserted at least four times per iteration of the inner loop, and it is being incurred even when no change occurs, e.g. on every ps or procstat -a or refresh of top.

In D27871#1101805, @des wrote:

I realize this is quite old, but I'd like to discuss the possibility of reverting 5844bd058aed, as it appears to have a significant performance impact which was never discussed at the time, while the benefit of the change appears to be purely hypothetical. I have no reason to doubt that pgrp_calc_jobc() is correct, but my own attempts to investigate possible errors in tracking pg_jobc did not uncover anything except an unrelated race condition in ppoll() and pselect() (cf. D47738 and D47741). I know of historical instances, e.g. D26116, but see no evidence of any unresolved issues at the time. Meanwhile, the cost of pgrp_calc_jobc() is quite high, especially in a debugging kernel where the process tree lock is being asserted at least four times per iteration of the inner loop, and it is being incurred even when no change occurs, e.g. on every ps or procstat -a or refresh of top.

I believe I gave up on trying to maintain counters by increments after jhb' report of failures (assertions were triggered) on some gdb tests. I.e. it should be added orphanage and exit process reparenting to the tests to trigger. I do not remember details, only that after several days of trying to handle it, I decided to recalculate the state instead of adjusting it.

Where do you see the issues with performance? Just in the assertion times, like sx_assert (due to WITNESS, perhaps)? Or is it the actual LIST_FOREACH() iterations in the pgrp_calc_jobc()?