Page MenuHomeFreeBSD

Fix mutual exclusion in pipe_direct_write().
ClosedPublic

Authored by markj on Jun 27 2019, 10:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 11 2024, 9:48 AM
Unknown Object (File)
Dec 20 2023, 4:31 AM
Unknown Object (File)
Dec 6 2023, 4:03 AM
Unknown Object (File)
Nov 12 2023, 6:27 PM
Unknown Object (File)
Nov 1 2023, 12:24 PM
Unknown Object (File)
Sep 27 2023, 11:20 PM
Unknown Object (File)
Aug 20 2023, 2:53 PM
Unknown Object (File)
Aug 11 2023, 2:28 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj added reviewers: kib, mjg.
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 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 marked 3 inline comments as done.

Apply feedback.

This revision now requires review to proceed.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.

This revision is now accepted and ready to land.Jun 28 2019, 5:10 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.

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.