sigtimedwait: Prevent timeout math overflows.
ClosedPublic
Actions

Authored by dchagin on Apr 20 2022, 2:29 PM.

Details

Reviewers

jhb
kib
mav

Commits

rG3130fd2daa5b: sigtimedwait: Prevent timeout math overflows.
rG4a700f3c329a: sigtimedwait: Prevent timeout math overflows.

Summary

Our kern_sigtimedwait() calculates absolute sleep timo value as 'uptime+timeout'.
So, when the user specifies a big timeout value (LONG_MAX), the calculated
timo can be less the the current uptime value.
In that case kern_sigtimedwait() returns EAGAIN instead of EINTR, if
unblocked signal was caught.

While here switch to a high-precision sleep method.

Below are to separate commits: fix and tests for it and for some other
kib@ commits to sigwait are.

Test Plan

Add tests for sigwait family syscalls.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 45250
Build 42138: arc lint + arc unit

Event Timeline

dchagin created this revision.Apr 20 2022, 2:29 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 20 2022, 2:29 PM

dchagin requested review of this revision.Apr 20 2022, 2:29 PM

Harbormaster completed remote builds in B45250: Diff 105207.Apr 20 2022, 2:30 PM

dchagin edited the summary of this revision. (Show Details)Apr 20 2022, 2:32 PM

dchagin added reviewers: jhb, kib, mav.

dchagin changed the repository for this revision from rS FreeBSD src repository - subversion to rG FreeBSD src repository.

mav added inline comments.Apr 20 2022, 4:02 PM

sys/kern/kern_sig.c
1280	Why not just: ts.tv_sec = INT32_MAX / 2; ? I don't see over used anywhere else.
1290	precision is uninitialized here. Probably not a problem, but not nice.

mav added inline comments.Apr 20 2022, 4:11 PM

sys/kern/kern_sig.c
1331	This may give false negative due to sbtt += tc_tick_sbt , while the msleep_sbt() below will use precise time. I guess it may end up in a tight loop till next hardclock.
1351	It would be nice to use this status instead of calling TIMESEL() another time. callouts never return prematurely.

fixed, thank you!

Harbormaster completed remote builds in B45256: Diff 105214.Apr 20 2022, 6:19 PM

dchagin marked 4 inline comments as done.Apr 20 2022, 6:28 PM

dchagin added inline comments.

sys/kern/kern_sig.c
1351	good catch! btw, as I understand, sbt eq 0 mean that the thread sleep indefinitely, while negative sbt mean thread no sleep. So, at least poll() implementation should be checked as seems it uses sbt -1 for infinity sleep timeout

kib added inline comments.Apr 21 2022, 12:24 AM

sys/kern/kern_sig.c
1351	Might be callouts indeed do not return prematurely, but note that msleep() uses process-shared wait chain. In other words, if more than one thread called sigtimedwait(2) syscalls, there could be aliased wakeups. Then, if other thread consumed the signal, shouldn't we re-iterate? Or should this case considered an application bug?

mav added inline comments.Apr 21 2022, 12:54 AM

sys/kern/kern_sig.c
1351	The removed chunk checked for error == EAGAIN (AKA EWOULDBLOCK) case -- specifically timeout expiration. In case of wakeup() call on the shared wait chain error should be zero and I see the code will indeed reiterate.

mav accepted this revision.Apr 21 2022, 12:56 AM

This revision is now accepted and ready to land.Apr 21 2022, 12:56 AM

dchagin marked an inline comment as done.Apr 21 2022, 9:45 AM

dchagin added inline comments.

sys/kern/kern_sig.c
1351	There will be a storm any way if there are more than one thread, but the threads that do not consume the signal fall to msleep again. At the same time, the sleep time is absolute and it is not necessary to recheck it as msleep will take care. Or Im missing something?