Page MenuHomeFreeBSD

Don't check rcv sockbuf limits when sending on a unix stream socket.
ClosedPublic

Authored by markj on Jul 30 2018, 5:29 PM.

Details

Summary

sosend_generic() performs an initial comparison of the amount of data
(including control messages) to be transmitted with the send buffer
size. When sending data with a control message, we then compare the
amount of data being sent with the amount of space in the receive buffer
size; if insufficient space is available, the data is lost. There are
at least two problems with this:

  • The size of control messages will typically change (increase) between these two checks. If the send and rcv socket buffer limits are equal, this means that the second check can fail even if the first one succeeds.
  • If the receive buffer is smaller than the send buffer, sending a message of size larger than the former and smaller than the latter will cause data loss. (This would seem to be an odd configuration, though.)

The first problem is easy to hit. Address it by removing the space
check in sbappendcontrol_locked() (note that sbappend_locked() omits the
check too).

We rely on the SB_STOP-based backpressure mechanism to ensure that
senders are blocked once the rcv buffer is full. With this change, it
becomes possible for the rcv buffer to contain data above its limit.
This doesn't violate any invariants in the socket layer, though, and
there is a bound on how much the limit can be exceeded. Given the
severity of the bug being fixed, I think this is a reasonable
compromise.

Note that there are still some bugs in this area; in particular, mbuf
allocation failures can cause data loss in some cases.

Test Plan
  • Regression test passes; see D16516.
  • Peter Holm's stress2

Diff Detail

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

Event Timeline

jbeich added inline comments.
sys/kern/uipc_sockbuf.c
958 ↗(On Diff #46038)

Looks incomplete, see

sys/kern/uipc_sockbuf.c:959:1: error: conflicting types for 'sbappendcontrol_locked'
sbappendcontrol_locked(struct sockbuf *sb, struct mbuf *m0,
^
sys/sys/sockbuf.h:144:5: note: previous declaration is here
int     sbappendcontrol_locked(struct sockbuf *sb, struct mbuf *m0,
        ^
sys/kern/uipc_sockbuf.c:987:9: error: assigning to 'int' from incompatible type 'void'
        retval = sbappendcontrol_locked(sb, m0, control);
               ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/kern/uipc_sockbuf.c
958 ↗(On Diff #46038)

Thanks, I had made a last-minute tweak before creating the review.

sys/sys/sockbuf.h
146 ↗(On Diff #46041)

This prototype should be deleted.

markj marked an inline comment as done.
  • Delete extra prototype.

I plan to commit the change this weekend if there are no objections.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2018, 8:27 PM
This revision was automatically updated to reflect the committed changes.