Remove deadlock in rc caused by pwait waiting for itself
ClosedPublic
Actions

Authored by lytboris_gmail.com on Jan 19 2021, 6:43 PM.

Details

Reviewers

melifaro
ae
0mp
freebsd_igalic.co

Group Reviewers

manpages

Commits

rG5bdce6ff546e: Remove deadlock in rc caused by pwait waiting for itself.

Summary

Imagine following situation:

Long time ago a_service was started through rc.d
We want to restart a_service and issue

service a_service restart

rc.subr reads current process PID (via file or process), sends TERM signal and runs pwait with PID harvested
a_service process dies very quickly so it's PID becomes available. It is possible that while original process was running PID counter overflowed and pwait can get a_service's PID as it's own PID trapping itself in a dealock.

This patch ignores pid(s) to wait that are equal to pwait PID.

Test Plan

Without patch proposed:

>while true; do pwait 24242 2>/dev/null; done
load: 4.60  cmd: pwait 19906 [running] 0.00r 0.00u 0.00s 11% 2120k
load: 4.60  cmd: zsh 20841 [running] 0.00r 0.00u 0.00s 12% 0k
load: 4.60  cmd: zsh 21720 [running] 0.00r 0.00u 0.00s 14% 0k
load: 4.60  cmd: zsh 6748 [running] 18810.01r 0.69u 6.41s 15% 7152k
load: 4.47  cmd: pwait 22730 [running] 0.00r 0.00u 0.00s 14% 2316k
load: 4.47  cmd: zsh 6748 [running] 18812.13r 0.72u 6.89s 15% 7152k
load: 4.47  cmd: pwait 23691 [running] 0.00r 0.00u 0.00s 16% 2172k
load: 4.47  cmd: pwait 23935 [running] 0.00r 0.00u 0.00s 17% 2172k
load: 4.47  cmd: zsh 24108 [running] 0.00r 0.00u 0.00s 16% 5624k
load: 4.47  cmd: pwait 24242 [kqread] 0.02r 0.00u 0.00s 17% 2300k
load: 4.47  cmd: pwait 24242 [kqread] 0.25r 0.00u 0.00s 17% 2300k
load: 4.47  cmd: pwait 24242 [kqread] 0.66r 0.00u 0.00s 15% 2300k

With patch proposed:

>while true; do ./pwait 64242 2>&1; done | fgrep -v 'No such process'
load: 11.65  cmd: zsh 60713 [running] 0.00r 0.00u 0.00s 12% 6960k
load: 11.65  cmd: zsh 55569 [running] 10.09r 0.17u 2.21s 15% 7152k
load: 11.36  cmd: pwait 62673 [running] 0.00r 0.00u 0.00s 15% 2236k
load: 11.36  cmd: zsh 55569 [running] 12.56r 0.20u 2.77s 16% 7152k
pwait: 64242: skip my own pid
load: 11.36  cmd: zsh 65110 [running] 0.00r 0.00u 0.00s 19% 7148k
load: 11.36  cmd: zsh 55569 [running] 16.07r 0.25u 3.56s 19% 7152k
load: 10.77  cmd: pwait 66246 [running] 0.00r 0.00u 0.00s 19% 2304k
load: 10.77  cmd: zsh 55569 [running] 17.96r 0.31u 3.95s 20% 7152k
load: 10.77  cmd: pwait 67423 [running] 0.00r 0.00u 0.00s 20% 2208k
^C

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

lytboris_gmail.com requested review of this revision.Jan 19 2021, 6:43 PM

lytboris_gmail.com created this revision.

lytboris_gmail.com edited the test plan for this revision. (Show Details)Jan 19 2021, 6:47 PM

rpokala added a subscriber: rpokala.Jan 19 2021, 7:32 PM

rpokala added inline comments.

bin/pwait/pwait.1
151	.Pp To avoid deadlock, .Nm will ignore its own pid, if it is provided as a process id to wait for.

lytboris_gmail.com updated this revision to Diff 82579.Jan 19 2021, 7:52 PM

Remember to bump .Dd at the top, to reflect the commit date. Other than that, looks good from a manpage standpoint.

This was reported before as https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218598 but I did not know for sure what to do so I left it alone. Since the patch is essentially the same, perhaps we should make this change. The wording of the error message in the patch in the PR seems a bit more formal, so better.

In D28240#631586, @jilles wrote:

This was reported before as https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218598 but I did not know for sure what to do so I left it alone. Since the patch is essentially the same, perhaps we should make this change. The wording of the error message in the patch in the PR seems a bit more formal, so better.

+1, although what I like about the patch by @lytboris_gmail.com is that it does not introduce a new variable.

This revision is now accepted and ready to land.Jan 20 2021, 1:06 PM

suggestion to improve warning

bin/pwait/pwait.c
150	i would say "skipping" here, it sounds a bit more like an English sentence (well, sentence fragment) this is because we are doing the skipping, we aren't instructing anyone to do it for us, and we're doing it right now