Page MenuHomeFreeBSD

Avoid creating unkillable processes with sendfile(SF_SYNC).
ClosedPublic

Authored by markj on Apr 20 2020, 1:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 26 2024, 12:49 PM
Unknown Object (File)
Dec 23 2023, 12:44 AM
Unknown Object (File)
Dec 4 2023, 8:18 PM
Unknown Object (File)
Sep 6 2023, 11:29 PM
Unknown Object (File)
Sep 6 2023, 11:23 PM
Unknown Object (File)
Sep 6 2023, 11:23 PM
Unknown Object (File)
Sep 6 2023, 11:22 PM
Unknown Object (File)
Sep 1 2023, 5:21 PM
Subscribers

Details

Summary

The SF_SYNC flag causes sendfile(2) to "sleep until the network stack no
longer references the VM pages of the file." This is implemented using
a struct sendfile_sync which is referenced by each mbuf cluster
containing file data, and sendfile(2) sleeps uninterruptibly until all
references are gone.

Suppose a single-threaded process uses sendfile(SF_SYNC) to transmit a
file over a unix socket pair that is private to the process (e.g.,
created with socketpair(2)). Then sendfile() will block until the data
is read from the receiving socket buffer, but this cannot happen unless
the socket is closed, so the process is unkillable.

Fix the problem by making the sendfile(2) wait interruptible. When the
wait is interrupted, sendfile's mbuf destructor callbacks are
responsible for freeing the structure. Make the implementation a bit
lighter by using a refcount (for sendfile_sync structure refs) and a
blockcount (to wait for mbuf references to decrement to zero), instead
of a mutex and condition variable.

Extend blockcount to support interruptible sleeps.

Test Plan

Reported and tested by pho.

Diff Detail

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

Event Timeline

markj added reviewers: glebius, kib.
kib added inline comments.
sys/kern/kern_sendfile.c
1204 ↗(On Diff #70802)

You will need blockcount merged to stable/12, or use cv_wait_sig() there. I think it is an argument to do it with _sig() one-liner first, and then commit this patch over.

sys/kern/kern_synch.c
408 ↗(On Diff #70802)

In fact it is EINTR or ERESTART.

This revision is now accepted and ready to land.Apr 20 2020, 2:53 PM
markj marked an inline comment as done.

_blockcount_sleep() can return ERESTART.

This revision now requires review to proceed.Apr 20 2020, 2:58 PM
This revision is now accepted and ready to land.Apr 21 2020, 3:54 AM
sys/kern/kern_sendfile.c
1204 ↗(On Diff #70802)

It is not sufficient to just convert to cv_wait_sig(): we will end up leaking the sendfile_sync structure if the wait is interrupted. I will commit the blockcount changes first, then prepare a patch that does not use blockcount, so it can be merged to stable/12.