Page MenuHomeFreeBSD

Fix mutual exclusion in pipe_direct_write().
ClosedPublic

Authored by markj on Jun 27 2019, 10:01 PM.

Details

Summary

We set PIPE_DIRECTW to indicate that a direct write (i.e.,
process-to-process copy) is pending on a pipe. When a reader has
copied the full contents of the wired pages, it clears PIPE_DIRECTW and
wakes up the writer, which proceeds to unwire the pages.

Direct writers must be serialized. pipe_direct_write() sleeps if
another thread has already set PIPE_DIRECTW. However, because the
reader clears PIPE_DIRECTW, it may wake up two writers and cause them to
race and access the page array concurrently.

Fix the problem by changing the read to not clear PIPE_DIRECTW.
Instead, a direct writer can determine whether the wired pages have been
read by checking pipe_map.cnt.

Test Plan

Only boot-tested so far.

The bug was found by syzkaller, and I am using it to test the diff.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Jun 27 2019, 10:01 PM
markj edited the test plan for this revision. (Show Details)Jun 27 2019, 10:04 PM
markj added reviewers: kib, mjg.
mjg added inline comments.Jun 27 2019, 10:48 PM
sys/kern/sys_pipe.c
972 ↗(On Diff #59119)

error is already zero on account of the previous check at 966, so there is no point in zeroing. also i don't think there is a need to start with error == 0 check. the body can be extended if (error) instead. this saves the initial branch always being true.

kib accepted this revision.Jun 27 2019, 11:11 PM
kib added inline comments.
sys/kern/sys_pipe.c
865 ↗(On Diff #59119)

Assert that PIPE_DIRECTW is set.

997 ↗(On Diff #59119)

There error must be zero, am I right ? If yes, perhaps assert it.

999 ↗(On Diff #59119)

I suggest adding asserts that we do not leak PIPE_DIRECTW on return from the function.

This revision is now accepted and ready to land.Jun 27 2019, 11:11 PM
markj updated this revision to Diff 59146.Jun 28 2019, 4:41 AM
markj marked 3 inline comments as done.

Apply feedback.

This revision now requires review to proceed.Jun 28 2019, 4:41 AM
markj added inline comments.Jun 28 2019, 4:41 AM
sys/kern/sys_pipe.c
865 ↗(On Diff #59119)

It may not be set when called from pipe_clone_write_buffer(), which also clears PIPE_DIRECTW. However, I believe we can safely postpone the clear until pipe_destroy_write_buffer() is called.

997 ↗(On Diff #59119)

We might have error == EPIPE.

999 ↗(On Diff #59119)

I added it only to the normal path. In the error1 case, another writer may have set PIPE_DIRECTW, so such an assertion would be incorrect.

mjg accepted this revision.Jun 28 2019, 5:10 AM
This revision is now accepted and ready to land.Jun 28 2019, 5:10 AM
kib accepted this revision.Jun 28 2019, 11:46 AM
kib added inline comments.
sys/kern/sys_pipe.c
997 ↗(On Diff #59119)

Indeed. You can assert EPIPE || 0, but I do not insist.

markj added a subscriber: pho.Jun 28 2019, 3:03 PM
pho added a comment.Jun 29 2019, 3:12 PM

I reproduced the problem and verified that this patch fixed the problems.
No other problems seen with this patch.

This revision was automatically updated to reflect the committed changes.