Page MenuHomeFreeBSD
Differential D23622

Do not read sigfastblock word on syscall entry.
ClosedPublic
Actions

Authored by kib on Feb 11 2020, 5:22 PM.
Tags
None
Referenced Files
F105958189: D23622.id68599.diff
Mon, Dec 23, 3:01 AM
F105944932: D23622.diff
Sun, Dec 22, 10:39 PM
Unknown Object (File)
Wed, Dec 18, 11:36 PM
Unknown Object (File)
Sat, Nov 30, 11:10 AM
Unknown Object (File)
Wed, Nov 27, 12:00 AM
Unknown Object (File)
Nov 22 2024, 11:36 AM
Unknown Object (File)
Nov 20 2024, 8:21 AM
Unknown Object (File)
Nov 17 2024, 1:48 PM
View All 95 Files
Subscribers
imp
pho

Details

Reviewers
jeff
mjg
Commits
rS358168: Do not read sigfastblock word on syscall entry.
Summary

Only read it on the attempt to deliver signal in ast(). If the word is set, signal is not delivered and word is kept, preventing interruption of interruptible sleeps by signals until userspace calls sigfastblock(UNBLOCK) which clears the word.

This way, the spurious EINTR that userspace can see while in critical section is on first interruptible sleep, if a signal is pending, and on signal posting. It is believed that it is not important for rtld and lbithr critical sections. It might be visible for the application code e.g. for the callback of dl_iterate_phdr(3), but again the belief is that the non-compliance is acceptable.
Most important is that the retry of the sleeping syscall does not interrupt unless additional signal is posted.

For now I added the knob to enable the word read on syscall entry to be able to diagnose possible issues due to spurious EINTR.

[This is not tested]

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib created this revision.Feb 11 2020, 5:22 PM
Herald added a subscriber: imp. · View Herald TranscriptFeb 11 2020, 5:22 PM
Harbormaster completed remote builds in B29296: Diff 68122.Feb 11 2020, 5:22 PM
kib updated this revision to Diff 68343.Feb 15 2020, 12:07 AM

Another approach, summary will be updated.

Harbormaster completed remote builds in B29392: Diff 68343.Feb 15 2020, 12:07 AM
kib retitled this revision from Convert usermode access (fueword) for sigfastblock(2) into a kernel read. to Do not read sigfastblock word on syscall entry..Feb 15 2020, 12:12 AM
kib edited the summary of this revision. (Show Details)
kib edited the summary of this revision. (Show Details)
jeff added inline comments.Feb 16 2020, 10:03 AM
sys/kern/kern_sig.c
160 ↗(On Diff #68343)

bool would be better.

Is this just for debugging?

sys/kern/subr_syscall.c
138 ↗(On Diff #68343)

predict_false? I'm afraid that this will go unused if we leave it in.

kib added inline comments.Feb 16 2020, 10:22 AM
sys/kern/kern_sig.c
160 ↗(On Diff #68343)

ok, I will change to bool.

It is for field diagnostic, I want to be able to ask users to flip the switch back without providing the patch, if I suspect an issue with spurious EINTR/ERESTART.

sys/kern/subr_syscall.c
138 ↗(On Diff #68343)

What do you mean by 'go unused' ?

kib updated this revision to Diff 68413.Feb 16 2020, 3:53 PM

Apply full pack of nano-optimizations, change sigfastblock_fetch_always to bool.

Harbormaster completed remote builds in B29429: Diff 68413.Feb 16 2020, 3:53 PM
kib updated this revision to Diff 68414.Feb 16 2020, 3:54 PM

re-upload with context.

Harbormaster completed remote builds in B29430: Diff 68414.Feb 16 2020, 3:54 PM
kib edited the summary of this revision. (Show Details)Feb 16 2020, 8:48 PM
kib updated this revision to Diff 68423.Feb 16 2020, 8:50 PM

One setpend() was missed after refactoring.

Harbormaster completed remote builds in B29433: Diff 68423.Feb 16 2020, 8:50 PM
jeff accepted this revision.Feb 16 2020, 8:53 PM

I would like to point out that to see the EINTR race the user would have to register a callback for rtld to call while not blocking signals itself and having registered a handler that then communicates information with the EINTR return case in the callback. I doubt this is even spec defined behavior and if linux is even weaker than we are they are more likely to just crash there.

I do not mind the fallback switch but it would be nice to garbage collect after some time.

This revision is now accepted and ready to land.Feb 16 2020, 8:53 PM
kib added a subscriber: pho.Feb 17 2020, 12:19 PM
pho added a comment.Feb 20 2020, 9:11 AM

I ran all of the stress2 tests without seeing any problems.

Closed by commit rS358168: Do not read sigfastblock word on syscall entry. (authored by kib). · Explain WhyFeb 20 2020, 3:34 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rS358168: Do not read sigfastblock word on syscall entry..

Revision Contents
Changeset List

PathSize
head/
sys/
kern/
4 lines
150 lines
8 lines
35 lines
sys/
6 lines

Diff 68599

View Options

head/sys/kern/kern_exec.c

Loading...
View Options

head/sys/kern/kern_sig.c

Loading...
View Options

head/sys/kern/subr_syscall.c

Loading...
View Options

head/sys/kern/subr_trap.c

Loading...
View Options

head/sys/sys/signalvar.h

Loading...