Page MenuHomeFreeBSD

Do not read sigfastblock word on syscall entry.
AcceptedPublic

Authored by kib on Tue, Feb 11, 5:22 PM.

Details

Reviewers
jeff
mjg
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
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 29433

Event Timeline

kib created this revision.Tue, Feb 11, 5:22 PM
kib updated this revision to Diff 68343.Sat, Feb 15, 12:07 AM

Another approach, summary will be updated.

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..Sat, Feb 15, 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.Sun, Feb 16, 10:03 AM
sys/kern/kern_sig.c
160

bool would be better.

Is this just for debugging?

sys/kern/subr_syscall.c
138–139

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

kib added inline comments.Sun, Feb 16, 10:22 AM
sys/kern/kern_sig.c
160

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–139

What do you mean by 'go unused' ?

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

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

kib updated this revision to Diff 68414.Sun, Feb 16, 3:54 PM

re-upload with context.

kib edited the summary of this revision. (Show Details)Sun, Feb 16, 8:48 PM
kib updated this revision to Diff 68423.Sun, Feb 16, 8:50 PM

One setpend() was missed after refactoring.

jeff accepted this revision.Sun, Feb 16, 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.Sun, Feb 16, 8:53 PM
kib added a subscriber: pho.Mon, Feb 17, 12:19 PM